-
Notifications
You must be signed in to change notification settings - Fork 327
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
Re-align toggle chevrons with right edge of sidebar #1562
Conversation
* wip * Style focus state in header nav * update focus ring style on all focussable elements * simplify * fix links in mobile sidebar overlay * put focus rings around a few more focusable elements * polish * update comment * review * better align focus ring on collapsible admonitions * comment and simplify sphinx-togglebutton focus ring * make css override more explicit * Fix SD link-card focus ring and on homepage, bring links inside card * Update docs/index.md --------- Co-authored-by: Daniel McCloy <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done self-reviewing!
@@ -23,11 +23,6 @@ | |||
font-size: var(--pst-sidebar-font-size); | |||
} | |||
|
|||
// Focus styles | |||
:focus-visible { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -147,22 +142,23 @@ | |||
// If it has children, add a bit more padding to wrap the content to avoid | |||
// overlapping with the <label> | |||
&.has-children { | |||
> .reference { |
There was a problem hiding this comment.
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.
padding-right: 30px; | ||
} | ||
} | ||
} | ||
// Navigation item chevrons | ||
label.toctree-toggle { | ||
position: absolute; |
There was a problem hiding this comment.
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.
position: absolute; | ||
top: 0; | ||
right: 0; | ||
flex: 0 0 auto; |
There was a problem hiding this comment.
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.
.toctree-collapsible-header { | ||
display: flex; | ||
justify-content: space-between; | ||
align-items: baseline; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For TOC entries that span two lines, this ensures that the chevron is aligned with the first line.
|
||
.toctree-collapsible-header { | ||
display: flex; | ||
justify-content: space-between; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This sends the link and chevron to opposite sides of the flex box.
@@ -209,7 +211,7 @@ nav.bd-links { | |||
} | |||
} | |||
|
|||
li > a { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
|
||
.current { | ||
> a { |
There was a problem hiding this comment.
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
@@ -367,6 +367,11 @@ def add_collapse_checkboxes(soup: BeautifulSoup) -> None: | |||
if not element.find("ul"): | |||
continue | |||
|
|||
header = soup.new_tag("div", attrs={"class": "toctree-collapsible-header"}) |
There was a problem hiding this comment.
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:
- Inconsistency (just because inconsistencies like these make me uneasy)
- 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.
There was a problem hiding this comment.
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.
@@ -192,6 +188,12 @@ | |||
right: 0em; // aligning chevron to the right | |||
} | |||
} | |||
|
|||
.toctree-collapsible-header { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
* Fix sidebar current notch * focus-ring-radius * missed a spot 0.125rem * keep focus ring on top
* restyle sphinx design tabs * increase panel border radius * increase line height, zero padding-y * use shadow variable * Update src/pydata_sphinx_theme/assets/styles/extensions/_sphinx_design.scss * Update src/pydata_sphinx_theme/assets/styles/extensions/_sphinx_design.scss
* docs: add instructions for custom SVG icons (pydata#1490) * docs: add instructions for custom SVG icons * docs: minor tweaks in SVG icon instructions * docs: some more tweaks to SVG icon instructions * Update docs/user_guide/header-links.rst Co-authored-by: Rambaud Pierrick <[email protected]> * Change literalinclude to code-block in header links * Update docs/user_guide/header-links.rst Co-authored-by: Daniel McCloy <[email protected]> * Update docs/user_guide/header-links.rst * Update docs/user_guide/header-links.rst * Update docs/user_guide/header-links.rst * Update docs/user_guide/header-links.rst * Update docs/user_guide/header-links.rst --------- Co-authored-by: tgresavage <[email protected]> Co-authored-by: Rambaud Pierrick <[email protected]> Co-authored-by: Daniel McCloy <[email protected]> * fix: make table background transparent (pydata#1546) * fix: make table background transparent * fix: make table background transparent * fix: add color-theme option to html tag (pydata#1536) * Silence warnings (pydata#1542) * avoid webpack warning during asset compile * avoid frozen modules warning during import * try to make jupyterlite quieter * add config option to silence warnings * fix tests * add docs * hide conditional warning logic in utils * bump: 0.14.2 → 0.14.3 * chore: back to dev * docs: add the list of component using a directive (pydata#1476) * fix: create the component list automatically * fix: read the first comment as documentation * docs: add disclaimer on .html suffix * docs: document every component with a simple one liner * fix: use regex to identify comments * update component branch (pydata#15) * Change default logo alt text (pydata#1472) * Default logo alt text only if no extra text * change default logo * use docstitle as default logo alt text * update docs to reflect change * Apply suggestions from code review Co-authored-by: Daniel McCloy <[email protected]> * use string formatting operator * Update docs/user_guide/branding.rst * docs fixes * Update docs/user_guide/branding.rst * add test * Update pyproject.toml * revert to original --------- Co-authored-by: Daniel McCloy <[email protected]> Co-authored-by: Rambaud Pierrick <[email protected]> * chore(i18n) catalan (pydata#1488) i18n: Translate sphinx.po in ca 100% translated source file: 'sphinx.po' on 'ca'. Co-authored-by: transifex-integration[bot] <43880903+transifex-integration[bot]@users.noreply.github.com> * Build(deps): Bump postcss and css-loader (pydata#1494) Bumps [postcss](https://github.com/postcss/postcss) to 8.4.31 and updates ancestor dependency [css-loader](https://github.com/webpack-contrib/css-loader). These dependencies need to be updated together. Updates `postcss` from 8.4.21 to 8.4.31 - [Release notes](https://github.com/postcss/postcss/releases) - [Changelog](https://github.com/postcss/postcss/blob/main/CHANGELOG.md) - [Commits](postcss/postcss@8.4.21...8.4.31) Updates `css-loader` from 3.6.0 to 6.8.1 - [Release notes](https://github.com/webpack-contrib/css-loader/releases) - [Changelog](https://github.com/webpack-contrib/css-loader/blob/master/CHANGELOG.md) - [Commits](webpack-contrib/css-loader@v3.6.0...v6.8.1) --- updated-dependencies: - dependency-name: postcss dependency-type: indirect - dependency-name: css-loader dependency-type: direct:development ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Revert "Build(deps): Bump postcss and css-loader" (pydata#1509) Revert "Build(deps): Bump postcss and css-loader (pydata#1494)" This reverts commit 185a37a. * Update pst docs buttons (pydata#1502) * call them button-links * copy edit * docs: link back to GitHub from PyPI metadata (pydata#1504) This will add a "Source" link in the PyPI page. * navigation_with_keys = False (pydata#1503) * navigation_with_keys = False * None -> False * Apply suggestions from code review --------- Co-authored-by: Daniel McCloy <[email protected]> * fix: convert "stable" to actual version number (pydata#1512) * convert "stable" to actual version number * fix tests re: navigation_with_keys * try bumping autoapi * refactor: use nbsphinx as the default execution lib (pydata#1482) * refactor: use nbsphinx as the default execution lib * add nbstripout to the pre-commits' * add pandoc to the readthedocs deps * refactor: clean the notebook * move the example to the correct folder * fix: solve link issue * install pandoc in the test environment * fix: display of large table in executed cells * avoid Userwarnings from matplotlib * hide the matplotlib wrning management cell * Update readthedocs.yml * build: use pandoc_binary to install pandoc * docs: add reference to pandoc in the setup * update docs * remove pypandoc_binary * Update pyproject.toml Co-authored-by: gabalafou <[email protected]> * ci: use back setup-pandoc * Trigger CI build --------- Co-authored-by: Gabriel Fouasnon <[email protected]> * BUG - Clear alt_text in conf.py (pydata#1471) * comment out alt_text in conf.py * set alt_text to empty string * remove alt_text from conf.py * fix: use 12rambau fork until it's merged with nikeee repo (pydata#1517) * deps: drop support for Sphinx 5 (pydata#1516) * remove ref to myst-nb * update minimal supported version of sphinx * Fix: (webpack.config.js) css-loader API change (pydata#1508) * Fix: (webpack.config.js) css-loader API change The build was broken in <https://github.com/pydata/pydata-sphinx-theme/commit/185a37aa36820f77bffa4c87a772092e9e7cc380>/<https://github.com/pydata/pydata-sphinx-theme/pull/1494>. This change fixes the build, and it seems to be in accordance with the current API as described at <https://github.com/webpack-contrib/css-loader/blob/c6f36cf91ac61743a70e81cfb077faa0f8730ebe/README.md#boolean>. Closes <pydata#1507>. * dedup * restore version bump --------- Co-authored-by: Daniel McCloy <[email protected]> * Fix duplicate HTML IDs (pydata#1425) * Fix duplicate HTML IDs * fix tests * Do not animate the version admonitions colors. (pydata#1424) Otherwise a delay has to be added to the accessibility color contrast checks, to wait for the colors to fully transition. * BUG - Remove redundant ARIA in breadcrumb navigation (pydata#1426) * style(i18n): French Typo fixed (pydata#1430) * Add the ability to add a center section to the footer (pydata#1432) * Add a center section for the footer * Add docs for footer_center * Add a test site for the center footer * test it in our own docs * remove new test site * add footer test --------- Co-authored-by: Daniel McCloy <[email protected]> * Build(deps): Bump actions/checkout from 3 to 4 (pydata#1433) Bumps [actions/checkout](https://github.com/actions/checkout) from 3 to 4. - [Release notes](https://github.com/actions/checkout/releases) - [Changelog](https://github.com/actions/checkout/blob/main/CHANGELOG.md) - [Commits](actions/checkout@v3...v4) --- updated-dependencies: - dependency-name: actions/checkout dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Add dropdown_text argument to generate_header_nav_html (pydata#1423) * Add dropdown_text argument to generate_header_nav_html * Add a test, fix typo in theme.conf and remove header_dropdown_text from docs/conf.py * fixed? --------- Co-authored-by: Daniel McCloy <[email protected]> * fix: rollback ref and Id changes (pydata#1438) * bump: version 0.13.3 → 0.14.0 (pydata#1440) * bump version * update version switcher * back to dev * fix: change the z-index of the dropdown (pydata#1442) In order to be on top of the primary sidebar on small screens. * fix: set the same background for dark/light (pydata#1443) * fix: set the same background for dark/light et the same background color for all state of the search field. It is currently only applied when hovered * fix: wrong css selector * Update src/pydata_sphinx_theme/assets/styles/components/_search.scss * Update src/pydata_sphinx_theme/assets/styles/components/_search.scss * Fix duplicate HTML IDs * fix tests * unique_html_id * backwards-compat generate_header_nav_html * feedback review * update fixture * ughhhh...caching * code cleanup * fix test snapshot * put comment inside def --------- Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: Denis Bitouzé <[email protected]> Co-authored-by: Stuart Mumford <[email protected]> Co-authored-by: Daniel McCloy <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Alenka Frim <[email protected]> Co-authored-by: Rambaud Pierrick <[email protected]> * chore: build the devcontainer automatically in codespace (pydata#1483) * chore: build the devcontainer automaticallyin codespace * refactor: lint * add pandoc to the environment * Fix font color in search input box (pydata#1524) * Fix color * Use --pst-color-text-base * docs: add DecentralChain (pydata#1528) Co-authored-by: jourlez <[email protected]> * Updates for file src/pydata_sphinx_theme/locale/en/LC_MESSAGES/sphinx.po in ru [Manual Sync] (pydata#1527) i18n: Translate sphinx.po in ru [Manual Sync] 96% of minimum 20% translated source file: 'sphinx.po' on 'ru'. Sync of partially translated files: untranslated content is included with an empty translation or source language content depending on file format Co-authored-by: transifex-integration[bot] <43880903+transifex-integration[bot]@users.noreply.github.com> * ignore transient errors in windows build CI (pydata#1520) * use warning list * clean up notebook * refactor to pass on all platforms? * simplify * fix logic * iterate backwards * fix plaform detection? also don't log unnecessarily�[H * ignore empty string warnings * remove notebook metawarning * Revert "remove notebook metawarning" This reverts commit 42f4672. * try again * debug the mysterious empty warning * escape color codes * import * triage by intermittency, not by platform; better var names * simplify * fix list.remove * undo what I broke * Update tests/utils/check_warnings.py * refactor: remove extention on component set-up (pydata#1529) * use event.key for search shortcut (pydata#1525) * use event.key for search shortcut * suggestions from review * caps lock --------- Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: gabalafou <[email protected]> Co-authored-by: Daniel McCloy <[email protected]> Co-authored-by: transifex-integration[bot] <43880903+transifex-integration[bot]@users.noreply.github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Ned Batchelder <[email protected]> Co-authored-by: Adam Porter <[email protected]> Co-authored-by: Denis Bitouzé <[email protected]> Co-authored-by: Stuart Mumford <[email protected]> Co-authored-by: Alenka Frim <[email protected]> Co-authored-by: Harutaka Kawamura <[email protected]> Co-authored-by: jourlez <[email protected]> * fix: use a directive instead of raw html * fix: make links externals * fix: set reference in paragraphs * fix: missing parameter * fix: use the stem for the component name * refactor: remove never used variables * standardize component descriptions --------- Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: gabalafou <[email protected]> Co-authored-by: Daniel McCloy <[email protected]> Co-authored-by: transifex-integration[bot] <43880903+transifex-integration[bot]@users.noreply.github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Ned Batchelder <[email protected]> Co-authored-by: Adam Porter <[email protected]> Co-authored-by: Denis Bitouzé <[email protected]> Co-authored-by: Stuart Mumford <[email protected]> Co-authored-by: Alenka Frim <[email protected]> Co-authored-by: Harutaka Kawamura <[email protected]> Co-authored-by: jourlez <[email protected]> * fix: primer link in docs (pydata#1556) * docs: add data-content (pydata#1559) Reproduce the change made in Sphinx 7 sphinx-doc/sphinx@8e730ae#diff-a5066e933cbf65adc46e0d1ab9a0b44e0a53ca64cc95dca7e6aa902aed6bd468R105 * Obviate background-from-color-variable (pydata#1558) * Obviate background-from-color-variable * backwards compatibility --------- Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: gresavage <[email protected]> Co-authored-by: tgresavage <[email protected]> Co-authored-by: Daniel McCloy <[email protected]> Co-authored-by: gabalafou <[email protected]> Co-authored-by: transifex-integration[bot] <43880903+transifex-integration[bot]@users.noreply.github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Ned Batchelder <[email protected]> Co-authored-by: Adam Porter <[email protected]> Co-authored-by: Denis Bitouzé <[email protected]> Co-authored-by: Stuart Mumford <[email protected]> Co-authored-by: Alenka Frim <[email protected]> Co-authored-by: Harutaka Kawamura <[email protected]> Co-authored-by: jourlez <[email protected]> Co-authored-by: Chris Holdgraf <[email protected]>
Not sure if this is related to the current PR, but just wanted to note that chevrons no longer seem to change their orientation based on whether a sub-menu is collapsed: It is hard to know if this is fixed because all this work is being done on a branch that isn't deployed to our |
@choldgraf I do have another PR that fixes this, but it's such a simple two-line fix, your comment made me think I might as well open a PR to fix this sooner rather than later: #1584 |
bdfa1dd
to
4839dc2
Compare
Superseded by #1582 |
Note: this PR is against
feature-focus
branch notmain
. Style fixes related to interaction state (focus, hover, current) will be collected in small increments intofeature-focus
before being merged intomain
.This PR introduces a few code changes:
.has-children
). Previously:li > a ~ input ~ label ~ ul
. Now:li > (div > a ~ label) ~ input ~ ul
. This was done so that the chevron label element would not have to be absolutely positioned.These changes are explained in greater detail in the inline comments.
Backstory
@jorisvandenbossche left a comment on #1549 showing chevrons not aligning with the sidebar edge.
This was the result of removing a
margin-right: -1rem
rule on thenav.bd-links
element. I removed that rule because it was causing the right edge of the focus rings to get clipped (the sidebar's overflow rules do not permit the part of the focus ring that goes outside the sidebar to be shown). The unintended consequence of removing the negative margin on the container was the chevrons no longer right-aligning with the edge of the sidebar.Test plan