Skip to content

Wrong alignment for custom icons #1839

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

Closed
PierreMarchand20 opened this issue May 29, 2024 · 10 comments · Fixed by #1846
Closed

Wrong alignment for custom icons #1839

PierreMarchand20 opened this issue May 29, 2024 · 10 comments · Fixed by #1846

Comments

@PierreMarchand20
Copy link
Contributor

I used the method described in #1554 to add custom icons. But with the 0.15.3, the alignement of the icons is broken:

image

vs

image

It seems to come from this css attribute:
image

vs

image
@drammock
Copy link
Collaborator

this change looks to have come in from #1545 via #1564. @gabalafou do you recall motivation for that change? I don't see notes about that particular change in the PR diff or conversation.

@PierreMarchand20
Copy link
Contributor Author

I confirm that changing baseline to center here

solves the issue.

@gabalafou
Copy link
Collaborator

I'm going to propose to fix the icon links in a different way. But @PierreMarchand20's suggestion raises a question about baseline versus center alignment. When the text links wrap to two or more lines, center alignment produces text that doesn't seem to be snapped to the same grid, as following screenshot shows:

But with baseline alignment, all the text is aligned to two imaginary horizontal lines:

Which one do we want? @smeragoel, thoughts?

@trallard
Copy link
Collaborator

Quick thought while killing some time: I prefer the text being aligned at the top (baseline) as it feels more intentional and easier to read vs center aligned - if there are long and short texts throughout it feels like my eyes jump all over the place

@smeragoel
Copy link
Contributor

I did some quick research and I'd also recommend using baseline alignment.

Pros of Baseline Alignment:

  • Consistency: All text aligns on the same line which makes the header look tidier.
  • Readability: Easier for users to read and scan, especially when text wraps to multiple lines.
  • Accessibility: Consistent alignment will help users follow the text more easily, reducing eye strain and cognitive load.

While there might be cases where center alignment looks better, it's less effective when there's a high chance of text wrapping. So I think we should go with baseline alignment.

@PierreMarchand20
Copy link
Contributor Author

The text does look better with baseline, but I personally prefer center for the icons. May be they could use two different attributs?

@drammock
Copy link
Collaborator

ahh, I didn't realize this was affecting both text and icons. I agree that doing icons at center and text at baseline makes a lot of sense

@PierreMarchand20
Copy link
Contributor Author

I mean, I did not check, I only assumed that it was the case since you were talking about it. But I may have misunderstood.

@gabalafou
Copy link
Collaborator

I created a pull request (#1846) that keeps the text links in baseline alignment and puts the icon links in center alignment.

@gabalafou gabalafou changed the title Wrong alignement for custom icons Wrong alignment for custom icons Jun 1, 2024
@PierreMarchand20
Copy link
Contributor Author

Thank you, it solved my issue!

drammock added a commit that referenced this issue Jun 4, 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.
3. 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.

---------

Co-authored-by: Daniel McCloy <[email protected]>
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 a pull request may close this issue.

5 participants