Skip to content

Re-align toggle chevrons with right edge of sidebar #1562

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.

Sign up for GitHub

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
wants to merge 9 commits into from
Closed

Re-align toggle chevrons with right edge of sidebar #1562

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
85badcb
Consistent focus ring (first pass) (#1549)
gabalafou Nov 13, 2023
2161070
fix regression: align toc toggle chevrons with right edge of sidebar
gabalafou Nov 15, 2023
885dffc
add some css comments
gabalafou Nov 15, 2023
920c91d
Resolve current sidebar link notch and focus ring (#1561)
gabalafou Nov 16, 2023
38ef477
Restyle Sphinx Design tabs (#1555)
gabalafou Nov 16, 2023
9803c5e
Fix tabbed panel colors (#1567)
gabalafou Nov 17, 2023
e054da1
update feature focus (#1569)
12rambau Nov 20, 2023
8754a6c
Merge branch 'feature-focus' into align-chevron
gabalafou Dec 1, 2023
dffe10f
Merge branch 'feature-focus' into align-chevron
gabalafou Dec 13, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 19 additions & 9 deletions src/pydata_sphinx_theme/assets/styles/sections/_sidebar-primary.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,22 +146,23 @@
// If it has children, add a bit more padding to wrap the content to avoid
// overlapping with the <label>
&.has-children {
> .reference {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is necessary given the change to the markup structure in toctree.py.

.reference {
padding-right: 30px;
}
}
}
// Navigation item chevrons
label.toctree-toggle {
position: absolute;
Copy link
Collaborator Author

@gabalafou gabalafou Nov 15, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Absolutely positioning the toggle chevron so that it lays on top of and right-aligned with the TOC link creates a hard-to-fix visual issue: when the user focus a link then hovers its chevron, the HTML box of the chevron turns a solid gray, and this solid gray box blocks out a portion of the focus ring, since the chevron's box is absolutely positioned to lay on top of the link's box.

If you try to give the link box a positive z-index or the chevron a negative z-index, in order to send it behind the focus ring so that it doesn't visually block the focus ring, then in that case the link's box is laid over the chevron's box and therefore the chevron can no longer receive hover or click events, rendering it useless.

I think it's better to not have these two elements stacked on each other and instead have them laid out side by side. I think that also sets a better stage for how the focus ring will move when we make the chevrons focusable and keyboard-operable. Because instead of the focus ring moving from surrounding the link + chevron to shrinking to surround the chevron, it will go from surrounding just the link, then move right to surround the chevron.

top: 0;
right: 0;
flex: 0 0 auto;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We don't want the chevron's box to be squished or expanded.


margin-right: -1rem; // align with right edge of the sidebar

height: 30px;
width: 30px;

cursor: pointer;

display: flex;
display: inline-flex;
justify-content: center;
align-items: center;

Expand Down Expand Up @@ -191,6 +192,12 @@
right: 0em; // aligning chevron to the right
}
}

.toctree-collapsible-header {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Is this a good classname?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think "heading" is better than "header" here. Also technically the primary sidebar nav is not really a toctree, so maybe sidebar-primary-collapsible-heading? Or something along those lines; tweak as needed to achieve similarity to related class names.

display: flex;
justify-content: space-between; // sends the link and collapse/expand chevron to opposite sides of the flex box
align-items: baseline; // for TOC entries that span two or more lines, ensures that the chevron is aligned with the first line
}
}

/* Between-page links and captions */
Expand All @@ -208,7 +215,7 @@ nav.bd-links {
}
}

li > a {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm a little bit concerned about this change. Mostly I'm concerned about downstream users of the theme who may have applied custom styles, copying our rules.

Copy link
Collaborator

Choose a reason for hiding this comment

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

it is needed because of the wrapping heading-name and chevron nodes into a flex container, right? I wouldn't worry about it too much --- we're not yet at v1.0 so we aren't guaranteeing stability (especially CSS/classnames) and if there were downstream overrides, they should be easy to fix (by copying this change) right?

li a {
display: block;
padding: 0.25rem 0.65rem;
@include link-sidebar;
Expand All @@ -222,13 +229,16 @@ nav.bd-links {
margin-left: 0.3em;
}
}
}

.current {
> a {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Another change due to the change in the markup tree in toctree.py

&.current {
@include link-sidebar-current;
background-color: transparent;
}

&:focus-visible {
border-radius: 0.125rem;
z-index: 10; // needed so that adjacent UI elements (such as the collapse toggle) don't partially cover the focus ring
}
}

// Title
Expand Down
7 changes: 6 additions & 1 deletion src/pydata_sphinx_theme/toctree.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -370,6 +370,11 @@ def add_collapse_checkboxes(soup: BeautifulSoup) -> None:
if not element.find("ul"):
continue

header = soup.new_tag("div", attrs={"class": "toctree-collapsible-header"})
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There are a couple things I don't like with this change.

It creates an inconsistency. TOC entries with children will have the following markup tree:

li
  input
  div
    a
    label
  ul

Whereas other TOC entries will look like:

li
  a

There is an additional level of depth in the tree, whereas previously all entries, whether they had children or not, had only one level of tree depth.

I'm worried about two things:

  1. Inconsistency (just because inconsistencies like these make me uneasy)
  2. Messing up consumers' sites because they customized the theme expecting a direct child relationship between the li and the link (or other elements like the chevron)

However, if I don't put the link and the chevron together in a row-flex div, then I don't know how to lay out the link and chevron in the same row without using absolute positioning, which as I mention in another inline comment, creates a hard-to-fix visual aberration.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I can see why this is troublesome. I guess you could wrap even the non-expanding headings in a similar div? As mentioned in another comment I think we shouldn't be too fretful about breaking downstream customizations as long as the fix is a straightforward one. Wrapping all headings in an extra div seems like it would facilitate those fixes being easier.

link = element.find("a")
element.insert(0, header)
header.insert(0, link)

# Add a class to indicate that this has children.
element["class"] = classes + ["has-children"]

Expand All @@ -388,7 +393,7 @@ def add_collapse_checkboxes(soup: BeautifulSoup) -> None:
if "toctree-l0" in classes:
# making label cover the whole caption text with css
label["class"] = "label-parts"
element.insert(1, label)
header.insert(1, label)

# Add the checkbox that's used to store expanded/collapsed state.
checkbox = soup.new_tag(
Expand Down