Migrate to Bootstrap 5x #631
Conversation
Coverage reportThe coverage rate went from None of the new lines are part of the tested code. Therefore, there is no coverage data about them. Diff Coverage details (click to unfold)server/apps/users/forms.py
|
|
Memo for myself: The scrollspy things are half-working now, they work once and then not when scrolling up again 🙈 |
|
I looked into the changes in colours for some texts, and those are a change in how BS treats it's colours (e.g. the info is now a lot more cyan than the muted colour it used to be). I can go ahead and edit those in our own |
|
okay, those colours are fixed :) |
|
Okay, I spent another 30 minutes on the scrollspy issue but to no avail 😂 |
|
This is amazing work @gedankenstuecke. Concerning scrollspy, I wonder if you could be hitting Bootstrap issue #36431. It looks like a known issue with 5.2. I tested out your changes with Bootstrap 5.1.3 and found the scrollspy worked considerably better. Would 5.1.3 be enough, at least until the fix is released? |
|
Yeah, I found that issue too and wondered if that's somehow related! The main trade-off between 5.1 and 5.2 is that only 5.2 comes with the dark mode options that at least partially motivated the wish to update bootstrap 🙈 If we wanted to stick with 5.2 we could either decide to up drop the scrollspy or replace it with a 3rd party option instead? |
|
Yes I see; that's a really good point. Even though it's frustrating, I'd say dropping it or replacing it would both be valid options. It shouldn't be hard functionality to replicate, but it might also be nice to be able to revert back to the Bootstrap version once it's fixed (to avoid pulling in unecessary dependencies). By the way, perhaps I'm misreading, but it looks like dark mode is 5.3 onwards (but still with the broken srollspy implementation). |
|
Ah yes, great catch, it's 5.3+ for the dark theme (and it's the one this PR is currently based on, so that works in principle!) I'd personally not be too sad about seeing the scrollspy at least temporarily go, especially as it's already disabled (or at least not applicable) for the mobile view where the menu bar hops to the top of the page. Once I have some time I can look into what an alternative to bs's scrollspy could be! |
|
I've removed the scrollspy for now, @llewelld it would be amazing if you had a look! |
|
@llewelld I know your official allocation to this has ended, but you had mentioned that you had a look at this already. I just wanted to check if you still wanted to submit that review or if someone else should have a go at it? 🙂 |
|
Thanks for prodding me about this @gedankenstuecke. I do have a partial review that I've not had a chance to complete yet. Sorry for this. I still plan to submit it, but do of course feel free to have others take a look as well (if I miss my chance to add my comments, that's on me). |
|
Thanks @llewelld, I'll see with @helendduncan if there's any reason that this would be urgent to get merged in! |
|
No hurry from my end @gedankenstuecke @llewelld I am only just back allocated on the project and my next task won't be held up by this, but am also happy to review if needed |
|
Great, let's give @llewelld a bit more time to finish his review in that case! David, it's much appreciated that you're taking the time for this! 🙏 |
There was a problem hiding this comment.
This is really nice. Apart from the menu highlighting which we discussed elsewhere (and which I agree isn't a big loss) things look very similar to the Bootstrap 4 version. Really nice.
There were a couple of points that I can't add as code comments so I'll put them here instead.
I experienced quite a serious scroll glitch on the shared stories page, caused by the 'smooth scroll' behaviour interacting with the code that resets the scroll position after a search. I've no idea why the Bootstrap change might have caused this. At any rate, forcing instant scroll fixed it for me:
diff --git a/static/js/shared-stories-filtering.js b/static/js/shared-stories-filtering.js
index 0c68d0d..9850060 100644
--- a/static/js/shared-stories-filtering.js
+++ b/static/js/shared-stories-filtering.js
@@ -43,7 +43,7 @@ $("#single_trigger_warnings :checkbox").change(function(){
document.addEventListener("DOMContentLoaded", function(event) {
var scrollpos = localStorage.getItem('scrollpos');
- if (scrollpos) window.scrollTo(0, scrollpos);
+ if (scrollpos) window.scrollTo({left: 0, top: scrollpos, behavior: "instant"});
});
window.onbeforeunload = function(e) {Completely separately, I found adding this helpful for testing. Although I appreciate this PR isn't supposed to fix everything about dark mode, this code will activate dark mode based on the OS/browser settings:
diff --git a/static/js/main.js b/static/js/main.js
index e69de29..f2d49b0 100644
--- a/static/js/main.js
+++ b/static/js/main.js
@@ -0,0 +1,10 @@
+$(function () {
+ function selectTheme(media) {
+ $("html").attr("data-bs-theme", media.matches ? "dark" : "light");
+ }
+
+ const media = window.matchMedia('(prefers-color-scheme: dark)');
+ selectTheme(media);
+ media.addListener(selectTheme);
+});
+Overall, the switch to Bootstrap 5 looks great to me.
|
|
||
| {% block content %} | ||
|
|
||
| <script src="{% static 'js/menu-alignment.js' %}"></script> | ||
|
|
||
|
|
There was a problem hiding this comment.
There's a small (0.5 rem) gap that's appearing between the heading menu item and the subsequent items; apparently added by the <h5>.
It looks okay actually, but I think is probably unintentional. Something like this could be used to remove it (added to main.css).
.list-group-item {
margin-bottom: 0;
}
There was a problem hiding this comment.
Great catch, I hadn't noticed. I've replaced the h5 with a more default declaration for the list view and made that bold instead, as I think that works well :)
| </div> | ||
| </form> | ||
| </form> | ||
| </div> | ||
| </div> | ||
| </div> |
There was a problem hiding this comment.
Something that becomes apparent in dark mode is that these cards have a border. It's barely visible in light mode but in dark mode it's really high contrast. But it highlights that there's no gap between the "Stories" card above and the "Review tabs" below.
It might be worth adding in some kind of gap, e.g. by adding in a top margin to the moderation-table-container:
.moderation-table-container {
margin-top: 3%;
margin-bottom: 3%;
}
There was a problem hiding this comment.
oh, amazing catch, added that!
Co-authored-by: David Llewellyn-Jones <[email protected]>
Co-authored-by: David Llewellyn-Jones <[email protected]>
…e/AutSPACEs into bootstrap_migration
|
Thanks a ton @llewelld, well spotted on those issues. I've implemented those fixes. And well done on getting the dark mode thing implemented right away! @helendduncan feel free to also give it a look and see if it works for you too! |
This will potentially be a bit of work as a lot of declarations have changed, but it would be good to migrate now rather than later in order to avoid being too outdated. This will also help with a lot of the accessibility improvements identified during the user tests (e.g. #605 & #603).
home.html