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.

Sign up for GitHub

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

Jump to bottom

Bump Bootstrap from 4.6.1 to 5.1.3 #681

Merged
merged 1 commit into from
Jul 15, 2022
Merged

Bump Bootstrap from 4.6.1 to 5.1.3 #681

merged 1 commit into from
Jul 15, 2022

Conversation

tnir
Copy link
Collaborator

@tnir tnir commented Jul 12, 2022 •
edited

What was the end-user problem that led to this PR?

Maybe nothing.

Closes #673

What was your diagnosis of the problem?

Newer library might cause faster page rendering.

What is your fix for the problem, implemented in this PR?

  • Update Bootstrap from 4.6.1 to 5.1.3.
  • .row classnames were removed at a bare minimum.

Why did you choose this fix out of the possible options?

Bootstrap 4.6 would be EOL'd sometime in the near future.

Signed-off-by: Takuya Noguchi takninnovationresearch@gmail.com

@deivid-rodriguez deivid-rodriguez temporarily deployed to bundler-site-bootstrap--c8yelt July 12, 2022 18:10 Inactive
@tnir tnir mentioned this pull request Jul 12, 2022
Closed
@deivid-rodriguez deivid-rodriguez temporarily deployed to bundler-site-bootstrap--c8yelt July 12, 2022 19:37 Inactive
@deivid-rodriguez deivid-rodriguez temporarily deployed to bundler-site-bootstrap--c8yelt July 12, 2022 22:40 Inactive
@deivid-rodriguez deivid-rodriguez temporarily deployed to bundler-site-bootstrap--c8yelt July 12, 2022 23:03 Inactive
@tnir
Copy link
Collaborator Author

tnir commented Jul 12, 2022

@duckinator Could you mind reviewing this while 1 day delayed from the pre-request at #644 (comment)?

@deivid-rodriguez deivid-rodriguez temporarily deployed to bundler-site-bootstrap--c8yelt July 12, 2022 23:06 Inactive
duckinator
duckinator reviewed Jul 13, 2022
View reviewed changes
Copy link
Member

@duckinator duckinator left a comment •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Everything looks good to me overall, but there's some commented-out @import lines that might be able to be removed. And thank you for documenting that contrast problem!

assets/stylesheets/application.css.scss Outdated Show resolved Hide resolved
assets/stylesheets/application.css.scss Outdated Show resolved Hide resolved
tnir
tnir commented Jul 13, 2022
View reviewed changes
Copy link
Collaborator Author

@tnir tnir left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@duckinator Thank for your review. Importing parts of Bootstrap was just updated. background-color for btn-primary/btn-primary:hover can be discussed more.

@deivid-rodriguez deivid-rodriguez temporarily deployed to bundler-site-bootstrap--c8yelt July 13, 2022 13:04 Inactive
tnir
tnir commented Jul 13, 2022
View reviewed changes
Copy link
Collaborator Author

@tnir tnir left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am trying this out...

assets/stylesheets/application.css.scss Outdated Show resolved Hide resolved
@tnir
Bump Bootstrap from 4.6.1 to 5.1.3
d6268af 
Signed-off-by: Takuya Noguchi <takninnovationresearch@gmail.com>
@deivid-rodriguez deivid-rodriguez temporarily deployed to bundler-site-bootstrap--c8yelt July 13, 2022 13:25 Inactive
@tnir tnir mentioned this pull request Jul 13, 2022
Closed
@tnir tnir requested a review from duckinator July 13, 2022 13:38
@tnir tnir self-assigned this Jul 13, 2022
duckinator
duckinator approved these changes Jul 13, 2022
View reviewed changes
Copy link
Member

@duckinator duckinator left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Everything looks good to me at this point. @tnir, is everything good to go on your end with this PR? If so, I'll merge it.

@tnir
Copy link
Collaborator Author

tnir commented Jul 13, 2022

@duckinator Yes. Thank you.

@duckinator duckinator merged commit 3ab155f into master Jul 15, 2022
@duckinator duckinator deleted the bootstrap-5.1.3-673 branch July 15, 2022 01:40
@tnir
Copy link
Collaborator Author

tnir commented Jul 15, 2022

Thank you again. Deploy confirmed at https://github.com/rubygems/bundler.github.io/runs/7350834575?check_suite_focus=true as well as in production https://bundler.io/.

This was referenced Jul 15, 2022
Closed
Merged
@deivid-rodriguez
Copy link
Member

I noticed a visual regression after this PR. The content is too wide and the margins too narrow, it looks worse the wider the screen is I believe. I reproduced in both Safari and Firefox.

Before

Captura de Pantalla 2022-07-16 a las 15 15 04

After

Captura de Pantalla 2022-07-16 a las 15 15 40

@deivid-rodriguez deivid-rodriguez mentioned this pull request Jul 16, 2022
Closed
@tnir tnir added dependencies Pull requests that update a dependency file javascript Pull requests that update Javascript code design architecture/legacy or broken Legacy and/or broken architecture labels Jul 23, 2022
@tnir tnir mentioned this pull request Jul 28, 2022
Merged
@tnir tnir mentioned this pull request Aug 27, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@duckinator duckinator duckinator approved these changes

@deivid-rodriguez deivid-rodriguez Awaiting requested review from deivid-rodriguez

@olleolleolle olleolleolle Awaiting requested review from olleolleolle

Assignees

@duckinator duckinator

@tnir tnir

Labels
architecture/legacy or broken Legacy and/or broken architecture dependencies Pull requests that update a dependency file design javascript Pull requests that update Javascript code
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Bump Bootstrap 4.6 to 5.1
3 participants
@tnir @deivid-rodriguez @duckinator