Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

build(deps): bump bootstrap from 4.4.1 to 5.5.3 #1375

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ReenigneArcher
Copy link
Contributor

@ReenigneArcher ReenigneArcher commented Sep 14, 2024

Notes:

  • popper.js also updated (required for updated bootstrap) (could also use bootstrap bundle and not include popper.js individually?) - edit: opted to use bootstrap bundle per discussion below
  • used jsdelivr for bootstrap
  • updated jquery (not sure if this is a dependency of bootstrap, but might as well update it), also moved it to jsdelivr for consistency - edit: opted to remove jquery per discussion below

Known Remaining Tasks:

  • general
    • hyperlinks are underlined now, they didn't use to be
    • content of pages are slightly wider now on full screen page (personally I think it looks better)
    • The <hr> separator underneath the home page title used to be black, now it's grey
  • navbar (mostly complete)
    • navbar-toggler (button) has slightly larger border radius than bs4... should this be changed to match the bs4 version or use the new default... edit: using new default (let me know if you want it changed)
    • when I hover over a dropdown menu item ("RESOURCES"), the text colour of the word seems to lag in its transition, or perhaps it has an animation, rather than immediately changing colour like the background changes colour immediately
    • When a menu dropdown is open (by clicking on it), the background colour of it should be a darker grey. Right now it's the same light gray that's identical to the hover colour
    • When a menu dropdown is open, and the mouse moves outside, then the hover state on the menu item completely disappears

And many unknowns to work review and work through.

Comparison sites

Original bs4: https://reenignearcher.github.io/beautiful-jekyll/
Updated bs5.3: https://reenignearcher.github.io/beautiful-jekyll-bs53-test/

@ReenigneArcher ReenigneArcher force-pushed the build/deps/bump-bootstrap-from-4.4.1-to-5.5.3 branch from a223b6e to 34c0e19 Compare September 14, 2024 22:15
@ReenigneArcher

This comment was marked as resolved.

@daattali
Copy link
Owner

  • Regarding popper: now that I think about it, is it even needed to include it? Can popper.js just be removed? If not, then using the bundle would be better than individually loading it
  • if I'm not mistaken, jquery is no longer required in BS5. If that's the case, then I would opt to remove jquery and translate the beautifuljekyll.js file to vanilla javascript. There's also some hacky code somewhere in the codebase that tries to ensure jquery isn't loaded multiple times, it'll be nice to remove that code. This may be a breaking change for some people who might rely on having jquery in their custom JS code, but the percent of people who use custom JS is very small, and I'm confident that they'll be able to look at the changelog and figure out this breaking change (this will need to be listed very clearly as a BREAKING CHANGE)
  • what do you mean by "content of pages is wider"? Did bs5 change the standard widths of the grid system?
  • navbar-toggler: can you post screenshots of before vs after

@ReenigneArcher ReenigneArcher force-pushed the build/deps/bump-bootstrap-from-4.4.1-to-5.5.3 branch from 34c0e19 to 37a6eac Compare September 20, 2024 23:02
@ReenigneArcher

This comment was marked as resolved.

@ReenigneArcher
Copy link
Contributor Author

ReenigneArcher commented Sep 20, 2024

I removed jquery in ba84b36

Note: It's still needed by staticman-comments.

Edit: didn't realize the staticman.js was part of this repo. I will update it as well.

Edit2: jquery removed from staticman.js here: 98188bb (I don't have a way to test this one)

Edit3: All commits merged into one now.

@ReenigneArcher ReenigneArcher force-pushed the build/deps/bump-bootstrap-from-4.4.1-to-5.5.3 branch from 247e79c to 98188bb Compare September 20, 2024 23:55
@daattali
Copy link
Owner

All sounds good. RE page width: what's the reason for the difference? Is a medium/large container simply a little wider now, or did they make the margins smaller, or ...?

@ReenigneArcher

This comment was marked as resolved.

@ReenigneArcher ReenigneArcher force-pushed the build/deps/bump-bootstrap-from-4.4.1-to-5.5.3 branch from 98188bb to 811cd87 Compare September 21, 2024 05:20
@daattali
Copy link
Owner

If it's possible for you to have two websites up, one for the old and one for the new bs version, to make comparing easy, that would be very helpful. If that's possible

@ReenigneArcher
Copy link
Contributor Author

If it's possible for you to have two websites up, one for the old and one for the new bs version, to make comparing easy, that would be very helpful. If that's possible

Original bs4: https://reenignearcher.github.io/beautiful-jekyll/
Updated bs5.3: https://reenignearcher.github.io/beautiful-jekyll-bs53-test/

@daattali
Copy link
Owner

Thanks! Looks very good. It seems like the extra width is because of the new xxl (> 1400 px) breakpoint. That's fine.

A few differences I noticed:

  • The <hr> separator underneath the home page title used to be black, now it's grey
  • navbar: when I hover over a dropdown menu item ("RESOURCES"), the text colour of the word seems to lag in its transition, or perhaps it has an animation, rather than immediately changing colour like the background changes colour immediately
  • When a menu dropdown is open (by clicking on it), the background colour of it should be a darker grey. Right now it's the same light gray that's identical to the hover colour
  • When a menu dropdown is open, and the mouse moves outside, then the hover state on the menu item completely disappears
  • You already know about the issue of links having underlines

@ReenigneArcher

This comment was marked as outdated.

@daattali
Copy link
Owner

The UI testing will also have to involve using all the different config parameters and the different YAML options

@ReenigneArcher ReenigneArcher force-pushed the build/deps/bump-bootstrap-from-4.4.1-to-5.5.3 branch 3 times, most recently from 47a1e07 to b6519ed Compare September 28, 2024 00:22
@ReenigneArcher
Copy link
Contributor Author

ReenigneArcher commented Sep 28, 2024

I've addressed these items, except:

when I hover over a dropdown menu item ("RESOURCES"), the text colour of the word seems to lag in its transition, or perhaps it has an animation, rather than immediately changing colour like the background changes colour immediately

Which I can't replicate. (Maybe it was fixed by addressing the hover issue).

Test site updated as well.

@daattali
Copy link
Owner

Have you tested it as thoroughly as you can, with all the different options and page types and browsers (mobile, safari, chrome, etc)? Is this ready for a final look over from me?

@ReenigneArcher
Copy link
Contributor Author

Have you tested it as thoroughly as you can

Not yet. Sorry, I'm a little slow with this as I'm working on several projects simultaneously.

with all the different options and page types

I've only built with the default config and pages available to it so far. If you have something specific you want tested please share it (I'm not an expert on all the options you have that would be affected by bootstrap)

Or if you have a repo that could have the updated theme applied I could fork it and cross check it that way.

Otherwise, looking through the config, my guess is these could possibly have some impact.

  • title-img
  • navbar-var-length

and browsers (mobile, safari, chrome, etc)?

I can test Firefox and Chrome on desktop and mobile. I can't easily test Safari (other than using the dev tools in Firefox... not sure how closely that emulates Safari)

Is this ready for a final look over from me?

Not yet, I will mark it as ready for review (and ping you) when I think it's 100% complete.

@daattali
Copy link
Owner

No rush, this definitely takes time (and testing can take even more time than coding!)

All the options that need to be tested are all the fields in the _config.yml file, and the YAML parameters that are described in the README.

Don't worry about safari, I can test on a friend's Mac if everything else seems fine.

@ReenigneArcher ReenigneArcher marked this pull request as ready for review October 20, 2024 14:23
@ReenigneArcher
Copy link
Contributor Author

@daattali sorry for the delay, but this is ready for review. I tested it as thoroughly as I can with different options. I only noticed one more difference not mentioned previously. Let me know if you want it addressed.

  • The navbar title and list items are slightly closer to the edge of the page. e.g. the title is slightly further left, and the most right side object is slightly further right.

@daattali
Copy link
Owner

I think links used to be underlined and now they are not?

@ReenigneArcher ReenigneArcher mentioned this pull request Oct 29, 2024
@ReenigneArcher ReenigneArcher force-pushed the build/deps/bump-bootstrap-from-4.4.1-to-5.5.3 branch from b6519ed to 4487c4a Compare October 29, 2024 17:39
@ReenigneArcher
Copy link
Contributor Author

Anything else needed for this?

@ReenigneArcher ReenigneArcher force-pushed the build/deps/bump-bootstrap-from-4.4.1-to-5.5.3 branch from 4487c4a to 3040f8f Compare November 7, 2024 23:44
@ReenigneArcher
Copy link
Contributor Author

I added the main changes to the changelog as well.

@daattali
Copy link
Owner

daattali commented Nov 9, 2024

I'm in the process of testing it.

@daattali
Copy link
Owner

daattali commented Nov 9, 2024

There are two small areas that need to be fixed:

  1. Underlines on links. I've noticed there are other places where there's underlines where they shouldn't be. Perhaps this needs to be tackled at the root, see if there's some sort of a CSS variable that is being used by bootstrap that changed between version 4 and 5? In any case, here are the 4 cases that I've noticed where the underlines need to be removed:

    • In the navbar, the title (left side) and the menu items all get underlines on hover
    • When you're on a blog post page, the buttons at the bottom "<-- Previous Post"/"Next Post -->", when you hover over them you'll notice an underline appearing between the text and the arrow
    • On the "/tags page, when you hover over any of the tag buttons, the text in the button gets underlined
    • If you use the YAML layout: minimal, all the links get underlined
  2. The cover image (cover-img) YAML parameter. It works fine when there is only one image. But there are two issues that need to be fixed when there's an array. I know you had to wrestle a lot with the removal of jQuery, it's alllllmost there. It'll be amazing when this is fixed and jquery is not needed!

    • To test the case of multiple images, you can use this YAML:
      cover-img:
        - /assets/img/thumb.png
        - /assets/img/crepe.jpg: This is a crepe 
      
    • When there is an array of images, the small caption box in the bottom right of the image should only be shown when an image has a caption. In this PR, whenever there are multiple images, the caption box is shown even when there's nothing in it
    • The fading of the image isn't quite right. Only the image should get faded, not the entire div (the title/subtitle/metadata/etc should not get faded). I remember this part took me a very long time to figure out 10 years ago, hopefully today with modern JS it won't be too difficult...

@daattali
Copy link
Owner

daattali commented Nov 9, 2024

@VincentTam it's been a while since I summoned you to this repo :) Would you be able to test this PR and verify that staticman still works?

@ReenigneArcher ReenigneArcher force-pushed the build/deps/bump-bootstrap-from-4.4.1-to-5.5.3 branch 4 times, most recently from c2fefae to 339b3ec Compare November 10, 2024 19:33
@ReenigneArcher
Copy link
Contributor Author

ReenigneArcher commented Nov 10, 2024

  • In the navbar, the title (left side) and the menu items all get underlines on hover

  • When you're on a blog post page, the buttons at the bottom "<-- Previous Post"/"Next Post -->", when you hover over them you'll notice an underline appearing between the text and the arrow

  • On the "/tags page, when you hover over any of the tag buttons, the text in the button gets underlined

Nice catch... I believe these items are resolved.

  • If you use the YAML layout: minimal, all the links get underlined

How do you want to handle this? Your minimal css file right now is very minimal.

Edit: I pushed a little change for this. Do you have an example minimal file I can test this with?

  • When there is an array of images, the small caption box in the bottom right of the image should only be shown when an image has a caption. In this PR, whenever there are multiple images, the caption box is shown even when there's nothing in it

This one is resolved. desc was null so added a check for that.

  • The fading of the image isn't quite right. Only the image should get faded, not the entire div (the title/subtitle/metadata/etc should not get faded). I remember this part took me a very long time to figure out 10 years ago, hopefully today with modern JS it won't be too difficult...

I have no idea to solve this one. Maybe the built in https://getbootstrap.com/docs/5.3/components/carousel/ should be used?

Edit: I believe this one is solved as well. I added z-index and position attributes to the css.

@ReenigneArcher ReenigneArcher force-pushed the build/deps/bump-bootstrap-from-4.4.1-to-5.5.3 branch from 339b3ec to bb32a3c Compare November 10, 2024 21:05
@ReenigneArcher ReenigneArcher force-pushed the build/deps/bump-bootstrap-from-4.4.1-to-5.5.3 branch from bb32a3c to 6ad1585 Compare November 12, 2024 03:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

bootstrap 5
2 participants