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 300 forks source link

use ellipsis to truncate breadcrumbs, and test with playwright #1861

Closed ivanov closed 4 days ago

ivanov commented 3 weeks ago

This PR:

joint work with @munkm and @drammock

closes #1583 closes #1568 lays groundwork for addressing #229

gabalafou commented 3 weeks ago

I love this work!

Also. Breadcrumbs. Hansel. Gretel. Hehe.

I took a stab a while back at using the trinity of text-overflow: ellipsis, overflow: hidden, and white-space: nowrap, and I remember being miffed at it not working. Then I started fiddling with the display: flex on the breadcrumb ancestors, but I couldn't get it to work, and I had other priorities, and so I gave up and moved on.

So I'm sure this pull request involved quite a bit of pulling at the strings to untangle the knot.

But there's still an issue. On a narrow mobile screen, if an intermediate (i.e., not last) breadcrumb is long, it will push off the screen:

screenshot showing long intermediate breadcrumb on iPhone SE

But I think I have the fix. It requires allowing the breadcrumbs as a whole to wrap, but truncating any breadcrumb whose line goes too long. Mind if I push the commit?

(I've accounted for making sure the breadcrumbs don't bleed into the article title, as shown in #1451.)

drammock commented 3 weeks ago

But I think I have the fix. It requires allowing the breadcrumbs as a whole to wrap, but truncating any breadcrumb whose line goes too long. Mind if I push the commit?

Go for it!

gabalafou commented 3 weeks ago

With my last push, the breadcrumbs for the same page in the above screenshot now takes three lines instead of one, but any individual breadcrumb part that takes more than one line (the middle line in this example) gets truncated with an ellipsis:

gabalafou commented 2 weeks ago

My personal feeling is that the breadcrumb should always be only one line, and we should, if necessary, elide multiple intermediate levels with ... and also truncate the final level with ....

Heh, I'm curious how we ended on completely opposite preferences. In my mind, the breadcrumbs are there to help me understand where I am in the hierarchy. Having intermediate ellipses somewhat defeats that purpose. In theory (or at least in my head), the breadcrumbs become even more important on mobile because you don't have the top nav and sidebars to help you stay oriented, but if a narrow screen and a constraint to keep it on one line causes even more elision, then I begin to wonder what purpose the breadcrumb component even serves on mobile.

In other words, to me the point of skipping some of the middle levels is to make it all fit on one line

Totally agree with you here, though. :)

drammock commented 2 weeks ago

Heh, I'm curious how we ended on completely opposite preferences.

I think mine is aesthetic: I don't like breadcrumbs as a means of navigation, so I think they should take up as little space as possible (to stay out of my way). But my preferences should not hamstring other users' ability to navigate sites. So after some further thought, I think we should definitely remove the intermediate ellipsis, ideally as part of this PR, so that folks who do use breadcrumbs can do so effectively.

If we do re-introduce the intermediate ellipsis later, we should probably only do so if we also make them click-to-expand-able, or perhaps add a theme option whether to enable intermediate breadcrumb elision. Since both of those directions introduce considerable complexity, there should probably be some brainstorming / community input first.

@gabalafou are you up for removing the intermediate ellipsis logic? If not I can try to get back to it sometime soonish.

gabalafou commented 2 weeks ago

Great, agree! I cannot get to the intermediates this week, but I could get to it next week

drammock commented 1 week ago

after some further thought, I think we should definitely remove the intermediate ellipsis, ideally as part of this PR

done in b2f2510

@gabalafou ready for review/merge. (also sorry for the rebase + force-push; I rebased while attempting to fix mysterious local test failures, which turned out to have been caused by something else entirely)

gabalafou commented 1 week ago

I think it's looking good. I pushed a commit to add margin, otherwise we lose the focus ring

Carreau commented 1 week ago

I'm guessing this shoudl wait for 0.15.4 right ?

Carreau commented 4 days ago

0.15.4 is out, merging.

gabalafou commented 4 days ago

Just wondering, were the tests added in this PR still passing after all of the changes we made? Seems odd...