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

Use navbar-nav class on text links only #1846

Merged
merged 3 commits into from
Jun 4, 2024

Conversation

gabalafou
Copy link
Collaborator

@gabalafou gabalafou commented Jun 1, 2024

This pull request fixes #1839 while also trying to simplify the way we use the Boostrap "navbar-nav" class.

Prior to this PR, we have both:

  1. "navbar-nav" class applied to text links rendered by "navbar-nav" template
  2. "navbar-nav" class applied to icon links rendered by "navbar-icon-links" template

This is what led to the bug in #1839 because I must have thought that the .navbar-nav selector was for the text links, not the icon links.

I see two possible ways of resolving this ambiguity:

  1. Keep the navbar-nav class on the text links, but remove it from the icon links. This requires adding some style rules to the icon links so they look the same with and without the class. This is the option shown in this PR.
  2. Keep the navbar-nav class on both sets of links, but change the name of the navbar-nav template to something else. This would require updating the docs, and I think it would also require theme adopters to update their theme config files, which is why I didn't choose this option.

@drammock
Copy link
Collaborator

drammock commented Jun 4, 2024

by default I can't see any difference in the navbars between this PR and the current dev site. But if I hack in a change via the dev console to make one of the svg tags for a navbar icon have style=height:"2em" then I can indeed see that the icons are center-aligned on the PR build. Looking into the CI failures now.

@drammock drammock merged commit 6a36d49 into pydata:main Jun 4, 2024
29 checks passed
@gabalafou gabalafou deleted the center-align-icon-links branch June 5, 2024 09:35
Carreau pushed a commit that referenced this pull request Jun 14, 2024
Part of #1865.

Fixes external issue
Quansight-Labs/czi-scientific-python-mgmt#94.

This pull request started as a simple increase of the hit area for the
navbar icon links, but then I realized that with #1846 having been
merged, I could do a little bit of code cleanup at the same time.
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.

Wrong alignment for custom icons
2 participants