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 ellipsis to truncate breadcrumbs #1583

Closed
wants to merge 4 commits into from

Conversation

drammock
Copy link
Collaborator

@drammock drammock commented Dec 5, 2023

WIP to address #1568. Doesn't work. The below proof-of-concept with the same tag structure / depth of nesting does work, so I'm a bit at a loss as to where the crucial difference is.

If anyone finds it helpful, the below HTML reconstructs the basic node structure of our theme. As written it doesn't work, but seems to work fine as long as you remove display: flex from any one of the outer flex nodes (anything parent to the ul node).

<html>
    <head>
        <style>
            body > div {
                background-color: yellow;
            }
            .FLEX {
                display: flex;
            }
            .UL {
                display: flex;
            }
            .LI {
                display: flex;
            }
            .LI::before {
                content: ">";
            }
            .LI:last-child {
                overflow:hidden;
            }
            .LI > span {
                overflow-x: hidden;
                text-overflow: ellipsis;
                min-width: 0;
                white-space: nowrap;
            }
        </style>

    </head>
    <body>
        <div class="FLEX"> <!-- bd-container -->
            <div class="FLEX">  <!-- bd-container__inner -->
                <main class="FLEX">
                    <div class="FLEX">  <!-- bd-content -->
                        <div class="FLEX">  <!-- bd-article-container -->
                            <div>  <!-- bd-header-article -->
                                <div class="FLEX"> <!-- header-article__inner header-article-items -->
                                    <div class="FLEX"> <!-- header-article-items__start -->
                                        <div>  <!-- header-article-item -->
                                            <nav>
                                                <ul class="UL">
                                                    <li class="LI">0123456789</li>
                                                    <li class="LI">ABCDE</li>
                                                    <li class="LI ACTIVE">
                                                        <span>abcdefg hijklmnop qrstuv wxyz</span>
                                                    </li>
                                                </ul>
                                            </nav>
                                        </div>
                                    </div>
                                </div>
                            </div>
                        </div>
                    </div>
                </main>
            </div>
        </div>
    </body>
</html>

@drammock drammock added the kind: bug Something isn't working label Dec 5, 2023
@drammock
Copy link
Collaborator Author

drammock commented Dec 6, 2023

ok, the feature works now on https://pydata-sphinx-theme--1583.org.readthedocs.build/en/1583/community/setup.html (personally I needed to zoom in a lot or turn on the browser dev tools pane to get the window narrow enough to see the effect; on smaller screens that shouldn't be necessary).

However, I had to remove display: flex from two container nodes in order to get this to work. This likely breaks something under some circumstances, but I'm not familiar enough with flex to predict which cases should be checked --- multiple content templates in the header is probably one case to look at, but there may be others. So this now works but needs review/testing. @ivanov do you have relevant expertise here?

@ivanov
Copy link
Contributor

ivanov commented Dec 6, 2023

This is fantastic, @drammock - thank you! My trick for checking is to inspect the breadcrumb node in questions and then paste its own content multiple times in there.

image

I'm setting this up locally right now to play with it, specifically I'm going to try to apply the logic to the breadcrumb-item class directly, without having to wrap inside an ellipsis span. But I'm a front-end impostor, at best, so we might have to cha-cha and back and forth by trying this and waiting to hear back about under which situation it breaks.

@drammock
Copy link
Collaborator Author

drammock commented Dec 7, 2023

I'm going to try to apply the logic to the breadcrumb-item class directly, without having to wrap inside an ellipsis span.

In my tests this didn't work, but maybe you'll have better luck (I think there was a SO post that explained why it wouldn't work but I'm on mobile and can't easily find it right now)

EDIT: https://css-tricks.com/flexbox-truncated-text/ (though cf https://codepen.io/psyrendust/pen/ZWPxmM/)

@12rambau
Copy link
Collaborator

12rambau commented Jan 3, 2024

It is working as expected on my side could you describe what is preventing you from making this PR ready to merge ?

@drammock
Copy link
Collaborator Author

drammock commented Jan 3, 2024

It is working as expected on my side could you describe what is preventing you from making this PR ready to merge ?

I haven't thoroughly tested that things don't look weird under circumstances that don't occur in our own docs / tests. Specifically (as I mentioned above) I'm worried about user sites that put multiple items into article_header_start and/or article_header_end (we use theme default which is only 1 item, the breadcrumbs, in article_header_start, and article_header_end is empty). As I write this, I realize that if someone put the breadcrumbs somewhere else besides the article header, then the ellipsis overflow might not work (putting breadcrumbs elsewhere is also not tested AFAIK).

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

// Font size slightly smaller to avoid cluttering up space too much
font-size: 0.8rem;

li.breadcrumb-item {
display: flex;
align-items: center;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

align-items was changed to center and was this conflicting, I resolved the conficlt.

@Carreau
Copy link
Collaborator

Carreau commented Jun 19, 2024

(this might get closed by #1861 anyway, but it was easy to fix conflict)

Carreau pushed a commit that referenced this pull request Jun 27, 2024
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.

joint work with @munkm and @drammock 

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

---------

Co-authored-by: Daniel McCloy <[email protected]>
Co-authored-by: Madicken Munk <[email protected]>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: gabalafou <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants