pydata / pydata-sphinx-theme

A clean, three-column Sphinx theme with Bootstrap for the PyData community
https://pydata-sphinx-theme.readthedocs.io
BSD 3-Clause "New" or "Revised" License
557 stars 299 forks source link

Wrong alignment for custom icons #1839

Closed PierreMarchand20 closed 1 month ago

PierreMarchand20 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

I confirm that changing baseline to center here https://github.com/pydata/pydata-sphinx-theme/blob/d7bbb79e444db1f4f4d6337c6fc76ad391eb1ece/src/pydata_sphinx_theme/assets/styles/sections/_header.scss#L89 solves the issue.

gabalafou commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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

Pros of Baseline Alignment:

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 commented 1 month ago

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

drammock commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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

PierreMarchand20 commented 1 month ago

Thank you, it solved my issue!