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

Infra: Make the color scheme toggle into a three-state cycler #2552

Merged
merged 11 commits into from
Apr 28, 2022
Merged

Infra: Make the color scheme toggle into a three-state cycler #2552

merged 11 commits into from
Apr 28, 2022

Conversation

flying-sheep
Copy link
Contributor

@flying-sheep flying-sheep commented Apr 25, 2022

Rendered

Fixes #2539. As described there, the only way to model the three states (manual light mode, manual dark mode, auto mode) is by having a tri-state UI. Nobody there had a problem with Furo’s behavior as that one guarantees visible theme changes on the first two clicks, and only has the third click change only the button (lightauto (light system theme) and darkauto (dark system theme))

I made it so the tooltip and aria label on the button tells people what a click will do. The only issue I see in my implementation is related to that: the tooltip/aria-label is incorrect once the system theme changes while the site is open. I can refactor everything so that can be easily fixed, but I first wanted to know if titles like this are welcome.

@flying-sheep flying-sheep requested a review from AA-Turner as a code owner April 25, 2022 10:20
Copy link
Member

@AA-Turner AA-Turner left a comment

Choose a reason for hiding this comment

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

I haven't looked at the logic in the JavaScript yet.

A

@AA-Turner
Copy link
Member

if titles like this are welcome.

Yes, it seems a good idea.

A

@hugovk hugovk added the infra Core infrastructure for building and rendering PEPs label Apr 25, 2022
@hugovk hugovk changed the title Make the button into a tri state one Infra: Make the button into a tri state one Apr 25, 2022
@flying-sheep
Copy link
Contributor Author

flying-sheep commented Apr 25, 2022

Yes, it seems a good idea.

OK, then I’ll change the logic so we can reset the label in a function passed to matchMedia(...).addEventListener()

/edit: done

@hugovk
Copy link
Member

hugovk commented Apr 25, 2022

Only checked functionality so far, but looking good!

https://pep-previews--2552.org.readthedocs.build/

There's a bit of oddness when hovering the mouse over the icon. Depending on which pixel it's hovering over, it'll either show the current state (.e.g "Light mode") or the icon action ("Switch to dark mode").

With Furo I only see the current state.

@flying-sheep
Copy link
Contributor Author

Ah I see, because the SVG has a <title/> which overrides the title attribute. I can just remove that.

@flying-sheep flying-sheep changed the title Infra: Make the button into a tri state one Infra: Make the color scheme toggle into a three-state cycler Apr 25, 2022
Copy link
Member

@CAM-Gerlach CAM-Gerlach left a comment

Choose a reason for hiding this comment

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

Thanks!

I tested the rendered preview through the different states, and on desktop and mobile, and everything appeared to look good and work as intended.

There was one noticeable UI/UX issue though—despite the button icon showing the current state, the tooltip shows the next state, which is confusing and does not match Furo's behavior, as @hugovk pointed out. IMO, it would make more sense for the tooltip to show the current state, to match the icon.

pep_sphinx_extensions/pep_theme/static/colour_scheme.js Outdated Show resolved Hide resolved
flying-sheep and others added 2 commits April 26, 2022 11:16
@flying-sheep
Copy link
Contributor Author

Fixed! Now the tooltips are very explicit.

@flying-sheep flying-sheep requested a review from AA-Turner April 28, 2022 15:37
@AA-Turner
Copy link
Member

I'll merge this now as it does fix a real issue -- if further issues arise we can fix them in follow-ups. Thanks!

A

@AA-Turner AA-Turner merged commit ebff37d into python:main Apr 28, 2022
@flying-sheep flying-sheep deleted the tri-state branch April 28, 2022 15:59
ccwang002 added a commit to ccwang002/ccwang002.github.io that referenced this pull request Jun 25, 2022
It recognizes user's prefer-color-scheme system setting and remembers
user's choice. Also it loads user's choice earlier so there won't be a
flash change of colors at start.

The implementation is largely based on
python/peps#2552 (MIT license),
whose source code locate at
https://github.com/python/peps/tree/f1af4a7c886cb9fb4bbb536923cccfd3b555a2ab/pep_sphinx_extensions/pep_theme
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
infra Core infrastructure for building and rendering PEPs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Theme toggle UI has no ability to return to default “follow system theme”
4 participants