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

Replace bootstrap-sass 3 to bootstrap 4.6.1 #644

Merged
merged 1 commit into from Jul 12, 2022

Conversation

tnir
Copy link
Collaborator

@tnir tnir commented Jul 6, 2022

before (1024-width on iPad Pro) after (1024-width on iPad Pro)
bundler io_(iPad Pro) bundler-site-tnir-boots-dje5be herokuapp com_(iPad Pro)

Closes #643

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

An end-user needs to load a legacy Bootstrap 3 just to browse the bundler docs.

What was your diagnosis of the problem?

No maintainers/contributors maintained the frontend of the site for 6 years.

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

HAML and SCSS would be easier for everyone to maintain as Bootstrap 4 is more recent version than 3.

Screenshots for navbar/searchbox

comment-1177209898

New introduction

Small improvement in searchbox spacing

Before this PR
before.mp4
After this PR
after.mp4

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

I cannot upgrade it from 3 to 5 (the latest major version as of creation of the PR) directly.

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

@tnir tnir force-pushed the tnir-bootstrap-4-643 branch 2 times, most recently from 943dcfb to 34a66ee Compare July 6, 2022 14:27
@deivid-rodriguez deivid-rodriguez temporarily deployed to bundler-site-tnir-boots-dje5be July 6, 2022 14:32 Inactive
source/v2.3/docs.html.haml Outdated Show resolved Hide resolved
source/shared/_whats_new.haml Show resolved Hide resolved
assets/stylesheets/application.css.scss Show resolved Hide resolved
assets/stylesheets/application.css.scss Show resolved Hide resolved
assets/stylesheets/_footer.scss Outdated Show resolved Hide resolved
assets/stylesheets/_docs.scss Outdated Show resolved Hide resolved
assets/javascripts/application.js Show resolved Hide resolved
@olleolleolle
Copy link
Member

(This is not a review, just appreciation: Your work on Bundler's docs is 🏆 super good.)

@deivid-rodriguez deivid-rodriguez temporarily deployed to bundler-site-tnir-boots-dje5be July 6, 2022 16:28 Inactive
@deivid-rodriguez deivid-rodriguez temporarily deployed to bundler-site-tnir-boots-dje5be July 6, 2022 16:30 Inactive
@deivid-rodriguez deivid-rodriguez temporarily deployed to bundler-site-tnir-boots-dje5be July 6, 2022 16:54 Inactive
@deivid-rodriguez deivid-rodriguez temporarily deployed to bundler-site-tnir-boots-dje5be July 6, 2022 17:02 Inactive
@deivid-rodriguez deivid-rodriguez had a problem deploying to bundler-site-tnir-boots-dje5be July 6, 2022 22:44 Failure
@deivid-rodriguez deivid-rodriguez temporarily deployed to bundler-site-tnir-boots-dje5be July 6, 2022 22:59 Inactive
@deivid-rodriguez deivid-rodriguez temporarily deployed to bundler-site-tnir-boots-dje5be July 6, 2022 23:03 Inactive
@duckinator
Copy link
Member

I can't give this a proper review right now, but I just wanted to say thank you so much for taking this on. It is greatly appreciated. ❤️

If nobody else gives this PR a proper review by next Monday (the 11th), feel free to @-mention me and I'll take a closer look at it.

@tnir
Copy link
Collaborator Author

tnir commented Jul 7, 2022

@olleolleolle Welcome and I just miss Turbinkanalen.

@duckinator Thanks a lot in advance for reviewing my upcoming PR for updating Bootstrap 4 to 5 or removal of it (not decided yet) hopefully I would create around July 11.

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.

You can improve two more structures around navbar.

source/layouts/_navbar.haml Outdated Show resolved Hide resolved
source/layouts/_footer.haml Outdated Show resolved Hide resolved
@deivid-rodriguez deivid-rodriguez temporarily deployed to bundler-site-tnir-boots-dje5be July 7, 2022 00:32 Inactive
@deivid-rodriguez deivid-rodriguez temporarily deployed to bundler-site-tnir-boots-dje5be July 7, 2022 00:50 Inactive
@tnir
Copy link
Collaborator Author

tnir commented Jul 7, 2022

before (1024-width on iPad Pro) after (1024-width on iPad Pro)
bundler io_(iPad Pro) bundler-site-tnir-boots-dje5be herokuapp com_(iPad Pro)

As of 058b6c9, .pull-rights were not replaced with float-right.

@tnir
Copy link
Collaborator Author

tnir commented Jul 9, 2022

All make sense to me as well.

Comments from anyone else than @deivid-rodriguez are now welcome again.

@tnir
Copy link
Collaborator Author

tnir commented Jul 11, 2022

/ping @duckinator

@duckinator
Copy link
Member

The only problem I see is that I get an unnecessary horizontal scrollbar, but I don't think that needs to block merging this. Aside from that this PR is good to go!

The scrollbar appears to be caused by the row class, and everything with that class is wider than my screen. Removing margin-right: -15px on .row fixes it, but I don't know if it's the actual cause of the problem:

image

image

@tnir would you rather I merge this as-is and open an issue for the scrollbar thing, or do you want to try to include the fix for that here? imo it's minor enough that I don't mind merging it and fixing that later.

@deivid-rodriguez deivid-rodriguez temporarily deployed to bundler-site-tnir-boots-dje5be July 11, 2022 22:46 Inactive
@deivid-rodriguez deivid-rodriguez temporarily deployed to bundler-site-tnir-boots-dje5be July 11, 2022 22:49 Inactive
Signed-off-by: Takuya Noguchi <takninnovationresearch@gmail.com>
@deivid-rodriguez deivid-rodriguez temporarily deployed to bundler-site-tnir-boots-dje5be July 11, 2022 22:58 Inactive
@tnir
Copy link
Collaborator Author

tnir commented Jul 11, 2022

@duckinator Good catch! While the original code looks non-standard, I fixed it in the latest commit so I think this would be ready to merge.

@duckinator
Copy link
Member

This looks good to me, so I'm merging it.

Thank you so much for all of your work on the site, @tnir!

@duckinator duckinator merged commit b157511 into rubygems:master Jul 12, 2022
@tnir tnir deleted the tnir-bootstrap-4-643 branch July 12, 2022 06:11
@tnir
Copy link
Collaborator Author

tnir commented Jul 12, 2022

Thanks @duckinator for a quick merging. This has blocked many tasks for me.

@deivid-rodriguez
Copy link
Member

I'm getting bad styles in what's new page, I guess it was after this PR.

Captura de Pantalla 2022-07-12 a las 14 01 08

@tnir
Copy link
Collaborator Author

tnir commented Jul 12, 2022

@deivid-rodriguez Yeah, I found this regression in last minute changes at #644 (comment) soon after this PR got merged.

@deivid-rodriguez
Copy link
Member

Thanks for fixing it!

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.

Escape from bootstrap-sass 3
4 participants