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

ivanov · 2024-06-05T07:11:05Z

This PR:

Fixes breadcrumb truncation to use CSS overflow: ellipsis
Provides an example for combining playwright and sphinx_build_factory to more thoroughly test our theme components.
Applies that approach to testing breadcrumb truncation via ellipsis when the breadcrumb is placed in various parts of our layout.

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

gabalafou · 2024-06-06T16:09:31Z

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 · 2024-06-06T18:11:07Z

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 · 2024-06-07T12:05:18Z

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

Left some self review comments

gabalafou · 2024-06-07T12:07:17Z

src/pydata_sphinx_theme/assets/styles/components/_breadcrumbs.scss

@@ -5,7 +5,7 @@ ul.bd-breadcrumbs {
  list-style: none;
  padding-left: 0;
  display: flex;
-  flex-wrap: nowrap;
+  flex-wrap: wrap;


I changed this line to allow the breadcrumb component as a whole to wrap across multiple lines. (But each individual crumb can take at most one line, given the following the rules.)

gabalafou · 2024-06-07T12:08:07Z

src/pydata_sphinx_theme/assets/styles/components/_breadcrumbs.scss

@@ -14,16 +14,12 @@ ul.bd-breadcrumbs {
    display: flex;
    align-items: center;
    white-space: nowrap;
-    flex-shrink: 0;


Instead of flex-shrink here, I use overflow-x to make sure that the breadcrumb part can take up at most one line

gabalafou · 2024-06-07T12:08:26Z

src/pydata_sphinx_theme/assets/styles/components/_breadcrumbs.scss


-    &.active {
-      flex-shrink: 1;


In my testing, this flex-shrink rule wasn't needed, so I removed it

gabalafou · 2024-06-07T13:20:23Z

src/pydata_sphinx_theme/assets/styles/components/_breadcrumbs.scss

+    a,
+    .ellipsis {


Now the text-overflow: ellipsis rule applies to all the breadcrumbs not just the last/current one

gabalafou · 2024-06-07T13:20:54Z

src/pydata_sphinx_theme/assets/styles/sections/_header-article.scss

-    align-items: start;
-    gap: 0.5rem;


These rules have no meaning after display: flex was deleted, so I deleted them.

gabalafou · 2024-06-07T13:21:44Z

src/pydata_sphinx_theme/assets/styles/sections/_header-article.scss

@@ -4,13 +4,6 @@
  // The items define the height so that it disappears if there are no items
  .header-article-item {
    min-height: var(--pst-header-article-height);
-    height: var(--pst-header-article-height);


Removing the height rule should prevent the issue in #1451, in which the breadcrumbs overlap the article title

gabalafou · 2024-06-07T13:22:05Z

tests/test_playwright.py

@@ -9,19 +9,6 @@
 playwright = pytest.importorskip("playwright")
 from playwright.sync_api import Page, expect  # noqa: E402

-# Important note: automated accessibility scans can only find a fraction of


Looks like this comment was accidentally pasted here

drammock

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:

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 .... But if we're going to take the approach here (of letting breadcrumb elements be on separate lines) then we should probably (?) disable the "skipping some levels in the middle" ellipsis:

In other words, to me the point of skipping some of the middle levels is to make it all fit on one line; if fitting on one line is no longer the goal, we shouldn't conceal intermediate levels.

Perhaps the ideal approach would be for the whole trail to be on one line, and the intermediate ... would be clickable, and when clicked, would expand the trail onto multiple lines.

All that said: unless @gabalafou fully agrees and wants to tackle that now, I'm OK to merge this as-is as a clear improvement, and transfer this comment to a new issue as a "possible future refinement"

gabalafou · 2024-06-11T17:36:09Z

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 · 2024-06-11T17:55:07Z

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 · 2024-06-11T18:03:10Z

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

Co-authored-by: Madicken Munk <[email protected]>

…wright.py co-authored-by: Madicken Munk <[email protected]> co-authored-by: Paul Ivanov <[email protected]>

…e line

drammock · 2024-06-17T19:53:32Z

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 · 2024-06-18T08:21:37Z

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

drammock

@gabalafou I'll let you merge if you're happy. Thanks @ivanov and @munkm !!

Carreau · 2024-06-19T12:50:10Z

I'm guessing this shoudl wait for 0.15.4 right ?

Carreau · 2024-06-27T09:58:31Z

0.15.4 is out, merging.

gabalafou · 2024-06-27T10:14:25Z

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

drammock · 2024-07-08T21:24:59Z

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

the late-breaking changes (letting each breadcrumb item be on its own line) are unlikely to have broken the tests because the test was extreme about setting very large (1440px) & very small (150 px) viewports to trigger the absence/presence of ellipsis.

drammock force-pushed the pi-breadcrumb-tests branch from c7bdc60 to 4ad3ecd Compare June 5, 2024 22:48

drammock marked this pull request as ready for review June 5, 2024 22:52

ivanov mentioned this pull request Jun 5, 2024

Broken breadcrumb on "The Jupyter Command" page jupyter/jupyter#702

Closed

drammock changed the title ~~breadcrumb tests~~ use ellipsis to truncate breadcrumbs, and test with playwright Jun 5, 2024

drammock requested a review from gabalafou June 5, 2024 22:57

gabalafou reviewed Jun 7, 2024

View reviewed changes

drammock previously approved these changes Jun 11, 2024

View reviewed changes

drammock and others added 14 commits June 17, 2024 13:21

add what we should need to shrink last flex element

158d4d4

the dubious change that makes ellipsis actually work

dea433a

release 0.15.3

ed65119

add initial failing test for breadcrumb expansion

5773228

initial breadcrumbs standalone test

96de332

Co-authored-by: Madicken Munk <[email protected]>

WIP breadcrumb site

f8b5ed7

fix all the things

5295653

rename

9c0f35d

refactor url_base into conftest.py; move non-a11y test into test_play…

1f7cae2

…wright.py co-authored-by: Madicken Munk <[email protected]> co-authored-by: Paul Ivanov <[email protected]>

[pre-commit.ci] Automatic linting and formatting fixes

1e969e4

remove cruft; rebase fixup

3857f62

Allow breadcrumbs to span multiple lines but truncate each part to on…

03f9eba

…e line

cleanups

fc335e7

don't elide middle levels of breadcrumb

b2f2510

drammock dismissed their stale review via b2f2510 June 17, 2024 19:44

drammock force-pushed the pi-breadcrumb-tests branch from 5fe965f to b2f2510 Compare June 17, 2024 19:44

drammock and others added 2 commits June 18, 2024 11:15

release 0.15.3

4c38a6f

Add margin to account for focus ring

2e4630f

drammock approved these changes Jun 18, 2024

View reviewed changes

Carreau mentioned this pull request Jun 19, 2024

use ellipsis to truncate breadcrumbs #1583

Closed

trallard added tag: CSS CSS and SCSS related issues tag: testing Issues related to PST testing labels Jun 25, 2024

Carreau merged commit 11e0411 into pydata:main Jun 27, 2024
29 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

ivanov commented Jun 5, 2024 •

edited by drammock

Loading

gabalafou commented Jun 6, 2024 •

edited

Loading

drammock commented Jun 6, 2024

gabalafou commented Jun 7, 2024 •

edited

Loading

gabalafou left a comment

gabalafou Jun 7, 2024

gabalafou Jun 7, 2024

gabalafou Jun 7, 2024

gabalafou Jun 7, 2024

gabalafou Jun 7, 2024

gabalafou Jun 7, 2024

gabalafou Jun 7, 2024

drammock left a comment

gabalafou commented Jun 11, 2024

drammock commented Jun 11, 2024

gabalafou commented Jun 11, 2024

drammock commented Jun 17, 2024

gabalafou commented Jun 18, 2024

drammock left a comment

Carreau commented Jun 19, 2024

Carreau commented Jun 27, 2024

gabalafou commented Jun 27, 2024

drammock commented Jul 8, 2024

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

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

Conversation

ivanov commented Jun 5, 2024 • edited by drammock Loading

gabalafou commented Jun 6, 2024 • edited Loading

drammock commented Jun 6, 2024

gabalafou commented Jun 7, 2024 • edited Loading

gabalafou left a comment

Choose a reason for hiding this comment

gabalafou Jun 7, 2024

Choose a reason for hiding this comment

gabalafou Jun 7, 2024

Choose a reason for hiding this comment

gabalafou Jun 7, 2024

Choose a reason for hiding this comment

gabalafou Jun 7, 2024

Choose a reason for hiding this comment

gabalafou Jun 7, 2024

Choose a reason for hiding this comment

gabalafou Jun 7, 2024

Choose a reason for hiding this comment

gabalafou Jun 7, 2024

Choose a reason for hiding this comment

drammock left a comment

Choose a reason for hiding this comment

gabalafou commented Jun 11, 2024

drammock commented Jun 11, 2024

gabalafou commented Jun 11, 2024

drammock commented Jun 17, 2024

gabalafou commented Jun 18, 2024

drammock left a comment

Choose a reason for hiding this comment

Carreau commented Jun 19, 2024

Carreau commented Jun 27, 2024

gabalafou commented Jun 27, 2024

drammock commented Jul 8, 2024

ivanov commented Jun 5, 2024 •

edited by drammock

Loading

gabalafou commented Jun 6, 2024 •

edited

Loading

gabalafou commented Jun 7, 2024 •

edited

Loading