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

Make hover effect on dropdown admonitions more prominent #1986

Closed
wants to merge 8 commits into from

Conversation

gabalafou
Copy link
Collaborator

@gabalafou gabalafou commented Sep 17, 2024

Closes Quansight-Labs/czi-scientific-python-mgmt#80

This PR:

  • Outlines the title bar of dropdown admonitions on hover
  • Darkens the chevron icon on hover
  • Restores the focus ring on the Sphinx Design dropdown admonitions by increasing the selector specificity for the focus ring, which fixes Focus ring on Sphinx Design dropdowns missing again #1923 (but I ultimately want to fix Sphinx Design)

Test plan

To test this PR, go to the following preview URL:

then hover your mouse over the dropdown admonitions. Try hovering over various parts of the admonition, for example, directly over the button versus anywhere in the dropdown title bar. Also check the hover effect on the admonition when it's in the expanded state, too.

An inconsistency

You may notice an inconsistency between the Sphinx Design collapsible admonitions and those from sphinx-togglebutton. With the SD admonitions, the chevron icon points right when collapsed, then rotates to point down when expanded. With sphinx-togglebutton, the icon points down when collapsed, then points up when expanded. But it's not within the scope of this PR to fix that inconsistency.

Copy link

github-actions bot commented Sep 17, 2024

Coverage report

This PR does not seem to contain any modification to coverable code.

Copy link
Collaborator

@drammock drammock left a comment

Choose a reason for hiding this comment

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

Two points:

  • all the admonitions on that page get a title underline when hovered (even the non-collapsible ones that aren't interactable). Is that intended? Seems misleading/confusing to have hover effects on non-interactible elements.
  • Collapsible admonition with no provided title gets a default "..." icon (sd-octicon-kebab-horizontal), which doesn't respond to hover in any way.

@trallard trallard added tag: accessibility Issues related to accessibility issues or efforts tag: component Issues or improvements associated with a given component in the theme labels Sep 19, 2024
gabalafou and others added 4 commits October 6, 2024 23:17
….po in ja (pydata#1979)

Co-authored-by: transifex-integration[bot] <43880903+transifex-integration[bot]@users.noreply.github.com>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
….po in fr (pydata#2000)

Co-authored-by: transifex-integration[bot] <43880903+transifex-integration[bot]@users.noreply.github.com>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
@gabalafou
Copy link
Collaborator Author

Thanks @drammock!

all the admonitions on that page get a title underline

Great catch! I didn't realize that the nested selector in _togglebutton.scss was casting such a wide net. I fixed this issue by changing .admonition to .dropdown.admonition.toggle.

In my latest batch of commits, I decided to try outlining the entire title bar on hover. I'm curious what folks think of this hover effect. cc @smeragoel

Copy link
Collaborator

@drammock drammock left a comment

Choose a reason for hiding this comment

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

In my latest batch of commits, I decided to try outlining the entire title bar on hover. I'm curious what folks think of this hover effect

Confused, I don't see any "outline" on hover (side note: when tabbing through the page I also don't see a focus ring on these dropdowns either). Tried both FF and Chromium on Linux.

One point I mentioned before is still true:

Collapsible admonition with no provided title gets a default "..." icon (sd-octicon-kebab-horizontal), which doesn't respond to hover in any way.

@trallard trallard added this to the 0.16.1 milestone Oct 15, 2024
@gabalafou
Copy link
Collaborator Author

@drammock somehow I forgot to git push 2f16458 🤦‍♂️

Please try this link again (be sure to clear your cache):

cc @smeragoel

PS. I'm aware of the missing focus ring, and I am working on a PR today to try to upstream some of the work to Sphinx Design.

@gabalafou
Copy link
Collaborator Author

gabalafou commented Oct 17, 2024

drammock
drammock previously approved these changes Oct 17, 2024
Copy link
Collaborator

@drammock drammock left a comment

Choose a reason for hiding this comment

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

@drammock somehow I forgot to git push 2f16458 🤦‍♂️

Please try this link again (be sure to clear your cache):

* https://pydata-sphinx-theme--1986.org.readthedocs.build/en/1986/user_guide/web-components.html

OK I see the hover-outline now.

PS. I'm aware of the missing focus ring, and I am working on a PR today to try to upstream some of the work to Sphinx Design.

I also see the focus ring though (your comments made me expect it to still not work)... Is &:focus:focus-visible a clever selector specificity hack that fixed it?

@gabalafou
Copy link
Collaborator Author

I also see the focus ring though (your comments made me expect it to still not work)... Is &:focus:focus-visible a clever selector specificity hack that fixed it?

Oh yes! I forgot that I added that in 2f16458 😅

@gabalafou
Copy link
Collaborator Author

gabalafou commented Oct 17, 2024

Two things came out of today's CZI Scientific Python team sync that need happen before merging this PR:

  • Add transition to the hover outline to make it a bit less jarring
  • Get an okay from @smeragoel

@smeragoel
Copy link
Contributor

@gabalafou This LGTM and is approved from my end!

Copy link
Collaborator

@drammock drammock left a comment

Choose a reason for hiding this comment

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

I like the added transition! I noticed one more tiny problem though. The very last dropdown on the page (in the toggle buttons section) doesn't get the extra inner space, so if it has tab focus and hover at the same time, the hover effect (purple) is mostly hidden behind the tab focus ring (pink). Presumably a CSS selector issue?

@dbitouze
Copy link
Contributor

dbitouze commented Oct 25, 2024

Wouldn't it be more appropriate for the outline to enclose not only the title, but also the entire admonition so that when the admonition is:

  • collapsed: the behavior is that of this PR (only the title is enclosed),
  • expanded: the frame encloses the entire rectangle of the admonition?

@drammock
Copy link
Collaborator

Wouldn't it be more appropriate for the outline to enclose not only the title, but also the entire admonition

Clicking on the content area doesn't trigger the collapsing though. Only clicking on the title bar. So IMO it would be misleading for the hover outline to encompass the entire card when it's expanded.

@dbitouze
Copy link
Contributor

Wouldn't it be more appropriate for the outline to enclose not only the title, but also the entire admonition

Clicking on the content area doesn't trigger the collapsing though. Only clicking on the title bar. So IMO it would be misleading for the hover outline to encompass the entire card when it's expanded.

I see. Agree.

@gabalafou
Copy link
Collaborator Author

After thinking through this some more, and in discussions with @Carreau, @trallard, and @smeragoel, I came to believe that the hover effect in this PR is too loud. Given other higher priority work, I'm going to close this PR for now, and we can revisit the hover effects on dropdown admonition at a later date.

I will carry over the work from this PR that restores the focus rings into a new and separate PR.

@gabalafou gabalafou closed this Nov 28, 2024
@gabalafou gabalafou deleted the chevron-hover branch November 28, 2024 21:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tag: accessibility Issues related to accessibility issues or efforts tag: component Issues or improvements associated with a given component in the theme
Projects
None yet
5 participants