search

meetecho / janus-gateway

Janus WebRTC Server
https://janus.conf.meetecho.com
GNU General Public License v3.0
8.25k stars 2.48k forks source link

Upgrade bootstrap and jquery versions #3295

Closed bkmgit closed 11 months ago

bkmgit commented 11 months ago

@lminiero

bkmgit commented 11 months ago

Separate pull request for documentation https://github.com/meetecho/janus-gateway/pull/3296

lminiero commented 11 months ago

Thanks! This looks like a huge refactoring, so I'll need time to review it.

From a quick glance, the change of bootstrap and font-awesome versions already involved many changes in the HTML code, but there's elements we sometimes create dynamically in the JavaScript sources, which assume the older versions are in use: that's the case, for instance, any time we create a new item with an icon somewhere, or when we create new divs dynamically. I expect some weirdness to happen when testing the demos more extensively. I'll try to do that in the next few days and let you know if I spot anything specific that needs fixing (here and in the docs PR).

lminiero commented 11 months ago
  • Colors have changed somewhat in bootswatch/5.4.2/cerulean/bootstrap.min.css from bootswatch/3.4.0/cerulean/bootstrap.min.css is it ok to have such changes?

Depends if the color scheme is completely different. If it is, we may have to switch to a different bootswatch template, at least or the sake of consistency. I'll have a look at that too.

lminiero commented 11 months ago

Mh it doesn't look like you changed anything related to boostrap, updating to v5... all pages look very bad, because none of the bootstrap styling is applying anywhere. This looks exactly like when I first started to experiment with boostrap v5.x myself. A proper update of version requires changing how the different types of divs are created: panels, menus, navbars, etc. The new version has a different syntax than v3, which is why all elements look like they have no styling at all.

As it is, this isn't an effort I could merge, sorry.

bkmgit commented 11 months ago

Updated main navbar, but have not changed other divs such as menus and panels. Can do so, but try to keep a similar look.

For a deployment see https://janus.kichakatokizito.solutions

screen shot of updated version in this pull request: NewBootstrap

screen shot of current version at https://janus.conf.meetecho.com/ : CurrentBootstrap

lminiero commented 11 months ago

screen shot of updated version in this pull request:

Yeah, and that already shows some styling not being applied (e.g., container and such). If you open the EchoTest demo, for instance, all elements are basically on top of each other (as they are in your screenshot too), panels aren't panels, and there's elements that should be hidden and aren't (the video box on the bottom). The navbar itself is actually broken, if you double check, since if you watch it on mobile there's no button that collapses the menu (which suggests it may be more a problem with menus than the navbar, but still it's an issue).

Unfortunately, as I explained, upgrading to bootstrap 5 is a huge endeavour that I tried already, and eventually gave up on for lack of time and because it didn't look like I was making (enough) progress. You're free to try as well (I may try again too, sooner or later, maybe going through bootstrap 4 first to see if migration guides help), but I have to anticipate that if the look and feel won't be the same as the demos we have now (at least functionally) we'll just stick with the devil we know and decline the PR.

lminiero commented 11 months ago

FYI, I've started attempting an upgrade to bootstrap 4, and it that "succeeds", I'll see how hard it is to move to 5 (I assume differences between 4 and 5 should be more manageable than those from 3 to 5). Should I get anywhere I'll post a new PR and notify you here.

bkmgit commented 11 months ago

Have a pull request for font awesome. https://github.com/meetecho/janus-gateway/pull/3298 Maybe easier to do this piece by piece. If of interest, can then try to upgrade Jquery or remove it entirely from everything apart from the documentation. It seems to only be used for an ajax request, which could be programmed directly. Newer versions of bootstrap do not use Jquery. Other change made is to move javascrpt loading after main html and css loading to give a better user experience. Waiting outcome of your bootstrap upgrade, no need to replicate that if you will do it.

lminiero commented 11 months ago

It seems to only be used for an ajax request, which could be programmed directly

We already use fetch, I think we have support for jquery's ajax stuff just as a fallback since it's what we used a long time ago, but I don't think anyone uses it anymore. We do make a heavy use of jquery for selectors though, in the demos code: I do realize it's something that can be done with plain JS, but it's also overly verbose and frankly a PITA. I'm sure a jquery-like wrapper can be done without using jquery, but I'm not that skilled of a web developer.

Anyway, please hold on to this until I have a PR ready for bootstrap 4/5. There will be a ton of code changes there already, and I'd rather make any further changes on top of that, rather than painfully merge any contribution back to that after the fact.

lminiero commented 11 months ago

PS: we're also currently using jquery.blockUI, which does require jquery at the moment. As such, any attempt to get rid of jquery for good, will also involve finding a proper replacement for that functionality.

Frenzie commented 11 months ago

PS: we're also currently using jquery.blockUI, which does require jquery at the moment. As such, any attempt to get rid of jquery for good, will also involve finding a proper replacement for that functionality.

Perhaps showModal() with some CSS trickery? Though I think that blockui thing is little more than an overlay with an element on top of it.

https://alexradulescu.github.io/freeze-ui/ seems to be written to be pretty much a drop-in replacement.

lminiero commented 11 months ago

https://alexradulescu.github.io/freeze-ui/ seems to be written to be pretty much a drop-in replacement.

I bumped into that one too, but it didn't look like it would fit the bill. We don't use BlockUI just to block the entire page, but also individual elements in some demos. That said, for the moment I don't plan to get rid of jQuery.