beautiful-jekyll icon indicating copy to clipboard operation
beautiful-jekyll copied to clipboard

build(deps): bump bootstrap from 4.4.1 to 5.5.3

Open ReenigneArcher opened this issue 1 year ago • 27 comments

  • Resolves #1138

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:

  • [x] general
    • [x] hyperlinks are underlined now, they didn't use to be
    • [x] content of pages are slightly wider now on full screen page (personally I think it looks better)
    • [x] The <hr> separator underneath the home page title used to be black, now it's grey
  • [x] navbar (mostly complete)
    • [x] 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)
    • [x] 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
    • [x] 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
    • [x] 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 avatar Sep 14 '24 22:09 ReenigneArcher

navbar-toggler (button) has slightly larger border radius than bs4... should this be changed to match the bs4 version or use the new default?

@daattali before I continue work on this PR, can you let me know your opinion on this?

ReenigneArcher avatar Sep 16 '24 23:09 ReenigneArcher

  • 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

daattali avatar Sep 20 '24 05:09 daattali

Regarding popper: now that I think about it, is it even needed to include it? Can popper.js just be removed?

It's needed for dropdowns in the navbar. See item 2 here: https://getbootstrap.com/docs/5.3/getting-started/introduction/#quick-start ... I'll go with the bundle method.

if I'm not mistaken, jquery is no longer required in BS5.

I'll try removing it and see if there's any repercussions. I saw the code you mention about not loading jquery twice, I'll remove that as well.

what do you mean by "content of pages is wider"? Did bs5 change the standard widths of the grid system?

There's not much difference, but it's slightly wider. I kind of like the new width better, as the old one often felt too narrow.

bs5.3: image

bs4.4: image

navbar-toggler: can you post screenshots of before vs after

The radius is just slightly larger.

bs5.3: image

bs4.4: image

ReenigneArcher avatar Sep 20 '24 23:09 ReenigneArcher

I removed jquery in https://github.com/daattali/beautiful-jekyll/pull/1375/commits/ba84b36315c864e70a6d9f0e98238eb7da4555f7

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: https://github.com/daattali/beautiful-jekyll/pull/1375/commits/98188bba57c36bc0ae23156a3b161d626df83db7 (I don't have a way to test this one)

Edit3: All commits merged into one now.

ReenigneArcher avatar Sep 20 '24 23:09 ReenigneArcher

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 ...?

daattali avatar Sep 21 '24 04:09 daattali

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 ...?

I'm not 100% sure. I assume something from here: https://getbootstrap.com/docs/5.0/migration/#grid-updates or maybe: https://getbootstrap.com/docs/5.0/migration/#rtl

ReenigneArcher avatar Sep 21 '24 04:09 ReenigneArcher

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

daattali avatar Sep 21 '24 05:09 daattali

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/

ReenigneArcher avatar Sep 21 '24 05:09 ReenigneArcher

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

daattali avatar Sep 25 '24 05:09 daattali

A few differences I noticed

Thanks, I updated the PR body and will start addressing these.

ReenigneArcher avatar Sep 25 '24 14:09 ReenigneArcher

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

daattali avatar Sep 26 '24 16:09 daattali

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.

ReenigneArcher avatar Sep 28 '24 00:09 ReenigneArcher

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?

daattali avatar Sep 29 '24 00:09 daattali

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.

ReenigneArcher avatar Sep 29 '24 01:09 ReenigneArcher

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.

daattali avatar Sep 29 '24 06:09 daattali

@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.

ReenigneArcher avatar Oct 20 '24 14:10 ReenigneArcher

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

daattali avatar Oct 22 '24 02:10 daattali

Anything else needed for this?

ReenigneArcher avatar Nov 07 '24 02:11 ReenigneArcher

I added the main changes to the changelog as well.

ReenigneArcher avatar Nov 07 '24 23:11 ReenigneArcher

I'm in the process of testing it.

daattali avatar Nov 09 '24 19:11 daattali

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 avatar Nov 09 '24 21:11 daattali

@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?

daattali avatar Nov 09 '24 21:11 daattali

  • 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 avatar Nov 10 '24 20:11 ReenigneArcher

I am now using this live in production (along with my other 2 PRs) at:

  • https://app.lizardbyte.dev
  • https://app.lizardbyte.dev/Sunshine

I will keep an eye out for issues.

ReenigneArcher avatar Jan 07 '25 23:01 ReenigneArcher

@ReenigneArcher can you allow me permission to edit the PR? Because it says there are conflicts, but I can't see what they are

daattali avatar Mar 16 '25 15:03 daattali

@daattali

I have that option disabled because I'm using this branch in production. But I've rebased it and fixed the conflicts in the changelog.

ReenigneArcher avatar Mar 16 '25 15:03 ReenigneArcher

Let me know if I can do anything to help get this completed. Probably if it doesn't get merged soon I'm going to have to start maintaining my own fork.

ReenigneArcher avatar Apr 20 '25 17:04 ReenigneArcher