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

Update deps #350

Merged
merged 5 commits into from
May 23, 2024
Merged

Update deps #350

merged 5 commits into from
May 23, 2024

Conversation

saitheninja
Copy link
Contributor

Hi. I was browsing the docs and I noticed that maybe I could help clean up some things.
Related issue: #228

Things done in this pull request:

  • Update Bootstrap from 5.0 beta to 5.3.3.
  • Remove old normalize.css. Bootstrap already includes its own reboot.
  • Remove jquery. It isn't a dependency of Bootstrap in v5. The bootstrap.bundle.js includes popper.js as a replacement.
  • Update nix-shell, because the ruby version was too old to build.
  • Update navbar to match previous styling. There's also some new CSS classes to target things better.
  • Update fonts. Serif fonts weren't being used at all, and Inter wasn't being loaded in some places. Also Lato isn't being used at all anymore, so I removed the old files.
  • Clean up some CSS.

Everything built fine on my machine. The only visual change is using the Inter font.
Screenshot_20240520_173925

Also there's some scripts that need to get their references updated to the correct filenames:
on the main site:
https://github.com/neovim/neovim/blob/40ce8577977fcdce8ad76863c70eb522e4cefd4d/scripts/gen_help_html.lua#L829
And the docs:
https://github.com/neovim/doc/blob/6f805e0d6085bd29ef881dcfe907638c943aac75/templates/report-header.sh.html#L11

I can do pull requests to update those if everything looks good, I just wanted to make sure that I didn't miss anything, or do something dumb. This pull request got a little bigger than I expected.

Problem:
Loading unnecessary files for bootstrap.

Solution:
Update bootstrap version and remove old files.
Bootstrap now includes popperjs. Doesn't need jquery anymore.
Bootstrap now includes its own reboot. Doesn't need normalize.css anymore.
Problem:
Ruby version in nix-shell is too old to build the site.

Solution:
Update ruby version and nix-shell commands.
Problem:
Lato font is no longer being used.
Inter font is not being loaded.
No difference between serif and sans-serif.

Solution:
Remove old files.
Update variables to load Inter font properly.
Use only sans-serif fonts.
Problem:
New Bootstrap update broke navbar styling.

Solution:
Update navbar to match previous styling.
Clean up a bit.
@justinmk
Copy link
Member

This is definitely wanted, but why are there 18k lines added?

@saitheninja
Copy link
Contributor Author

Good question. I think that's mostly the unminified versions of bootstrap.css and bootstrap.bundle.js.

I included the unminified versions because it's easier to browse through without having to load up a dev environment. Also the repo currently has the unminified and minified versions so I was just pattern matching. But I dont have any strong feelings about including it.

Should I go ahead and remove the unminified files?

@justinmk
Copy link
Member

well, if that's the case, can the old ones be removed?

@saitheninja
Copy link
Contributor Author

Sure. Give me like 30 minutes and I'll clean it up.

Problem:
Don't need unminified versions.

Solution:
Remove unminified versions.
@saitheninja
Copy link
Contributor Author

Okay, cleaned it up. The diff is now much more reasonable.

Also made pull requests to update the links from the other repos to point to the correct files.

@clason clason requested a review from justinmk May 22, 2024 07:26
}
}

body {
/* font-family: "Helvetica Neue", Helvetica, Calibri, Arial, sans-serif; */
font-family: var(--serif-fonts);
font-family: var(--sans-serif-fonts);
Copy link
Member

Choose a reason for hiding this comment

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

We definitely weren't using serif, I wonder why this was like this.

Copy link
Member

Choose a reason for hiding this comment

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

I confirmed that this matches the file from https://getbootstrap.com/docs/5.3/getting-started/download/

Copy link
Member

@justinmk justinmk left a comment

Choose a reason for hiding this comment

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

This is extremely helpful. Keeping up with details like "normalize.css vs reboot" takes attention. Thank you!

@justinmk justinmk merged commit 5d2c0e1 into neovim:master May 23, 2024
justinmk pushed a commit to neovim/doc that referenced this pull request May 23, 2024
Problem:
Not using minified version of bootstrap dep.
Using wrong font.
See neovim/neovim.github.io#350

Solution:
Update link to bootstrap CSS file.
Remove refs to old font.
justinmk pushed a commit to neovim/neovim that referenced this pull request May 23, 2024
Problem:
Not using minified version of bootstrap.
Don't need to load normalize with new version of bootstrap.
See neovim/neovim.github.io#350

Solution:
Update link to bootstrap file.
Remove link to normalize.
huangyingw pushed a commit to huangyingw/neovim that referenced this pull request May 31, 2024
Problem:
Not using minified version of bootstrap.
Don't need to load normalize with new version of bootstrap.
See neovim/neovim.github.io#350

Solution:
Update link to bootstrap file.
Remove link to normalize.
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.

None yet

2 participants