Skip to content

Upgrade to Bootstrap v5 #780

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

Merged
merged 24 commits into from
Mar 22, 2025
Merged

Upgrade to Bootstrap v5 #780

merged 24 commits into from
Mar 22, 2025

Conversation

franpb14
Copy link
Collaborator

@franpb14 franpb14 commented Feb 22, 2025

Main points

  • I have tracked almost all the breaking changes using this excel https://docs.google.com/spreadsheets/d/135pGhEJB-vwUCRqX88EIE9KftdF-uJAAGPMOM09SMrc/edit?gid=0#gid=0
  • take special attention to this commit, In bootstrap 3 a fontsize of 14px was applied but after upgrading there was no font-size so the 62.5% was being applied, so the font-size was very small and I had to change not only the html but the elements using rem.
  • there are some changes in javascript because it dropped jQuery
  • there are some things that are not exactly like in bootstrap 3, perhaps the most obvious is the secondary button which is gray now instead of white, should we change these things so they are as before?

@franpb14 franpb14 marked this pull request as draft February 22, 2025 13:54
@franpb14 franpb14 marked this pull request as ready for review February 28, 2025 18:04
@markets
Copy link
Collaborator

markets commented Mar 3, 2025

@franpb14 couple of minor issues I just noticed:

  • there is no icons rendering in the /admin section
  • the top navbar in mobile is very small, it needs more height

About this point:

there are some things that are not exactly like in bootstrap 3, perhaps the most obvious is the secondary button which is gray now instead of white, should we change these things so they are as before?

No need to change it, IMO the current defaults are totally fine for now. I'd like to make some overall adjustments (buttons, inputs, ...) but in future iterations.

@markets
Copy link
Collaborator

markets commented Mar 5, 2025

@franpb14 just made another round of 👀 and I think overall looks quite stable now, the only thing that it works a bit weird is the top navbar in mobile:

Captura de pantalla 2025-03-05 a las 20 03 53
  • It needs a bit more height
  • Some breakpoints, like the one above ☝🏼, seems to broke the functionality

@markets
Copy link
Collaborator

markets commented Mar 10, 2025

@franpb14 The navbar is still not working well:

  1. Desktop
Captura de pantalla 2025-03-10 a las 13 42 59
  1. Mobile
    https://github.com/user-attachments/assets/fef0cde1-67ac-4bd4-9b97-e096f07dc810

@markets markets changed the title Upgrade to latest bootstrap version Upgrade to Bootstrap v5 Mar 11, 2025
@markets
Copy link
Collaborator

markets commented Mar 14, 2025

Looks good now 👏 I'd probably push some tweaks 💅, but later in a different branch.

I think this is ready to be merged, I'm just a bit worried about the Docker build crash. That's the same script we are using to deploy the app, so right now we cant probably deploy it 🤣

@sseerrggii
Copy link
Contributor

@markets should I ask PokeCode about the docker problem?

@markets
Copy link
Collaborator

markets commented Mar 17, 2025

@sseerrggii Would be nice 👌🏼 if they can help us to understand why this is failing and how we can fix it.

@markets markets force-pushed the upgrade-bootstrap branch from 8844fe5 to afb1233 Compare March 22, 2025 15:23
@markets
Copy link
Collaborator

markets commented Mar 22, 2025

@franpb14 @sseerrggii going to merge this now, the Docker build failure seems unrelated at all, as it's also happening in another branches. We'll need to fix that of course, but in the meantime we can continue iterating on develop.

@microstudi

@markets markets merged commit 6119ffe into develop Mar 22, 2025
5 of 10 checks passed
@markets markets deleted the upgrade-bootstrap branch March 22, 2025 15:31
@markets markets mentioned this pull request Mar 23, 2025
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.

3 participants