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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Upgrade to Bootstrap 5.1 #135

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

Conversation

GuitarBro
Copy link

First PR here, so go easy on me. 馃槈

This one's fairly self-explanatory. I upgraded the Bootrstrap version from 3 to 5.1, attempting to preserve the general layout/content while making things look more modern.

Notable changes:

  • Removed the dependency on FontAwesome 3.2.1 since Bootstrap 5 has their own icons.
  • Removed the dependency on ekko lightbox as it was no longer being maintained beyond Bootstrap 4.x
    • In it's place is a simple lightbox replacement made with vanilla js. It should have all the existing functionality, though not the complete functionality of ekko lightbox. As a tradeoff, it is much more mobile-friendly
  • Made compat more mobile-friendly, though on very small devices it may still require some horizontal scrolling.
  • Updated the homepage banner to be full-width and behind the hero text. Personally I think the look is an improvement, but I understand this may be a matter of taste. 馃槢

Other things to mention: I'm not sure if there's any reason to have the jQuery dependency anymore since Bootstrap 5.x no longer requires it and ekko lightbox is gone. I left it just in case, but if those were the only places it was used, perhaps that should be removed as well to reduce page load times.

Pretty confident I got all the pages that needed attention, but I may have missed something so let me know if I need to revisit this.

Side note: CTRL/CtrlSTheWorld/GuitarBro all are me, just in case there's any confusion. Sorry about the inconsistency across git/github.

@leoetlino
Copy link
Member

This is a little bit risky because old articles might be relying on Bootstrap CSS (for responsive embeds, and maybe layout and centering utils like text-center, float/pull-right, etc.) This will need to be carefully tested to ensure articles don't break.

@GuitarBro
Copy link
Author

@leoetlino agreed. Is it possible to get a copy of the article data locally? I'd be happy to give it a look through for compatibility issues, I just lack the means to do it.

@GuitarBro
Copy link
Author

Any chance I could get some visibility on this? It looks like #140 implements a dark theme that would be incompatible with Bootstrap 5.1 which is unfortunate. I was planning to work on implementing one of the compatible themes from bootstrap-dark-5 once this was approved, but without a way to verify the condition of the existing articles, this can't go in yet.

Perhaps since #140 has no conflicts, that one should be merged for the time being with a plan to eventually replace it with a bootstrap-dark-5 theme once this gets merged? Alternatively, I can work on adding it to this pull request if that makes more sense.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants