-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
feat: Removed theme buttons and replaced them with radix switch #1417
Conversation
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.
PR Compliance Checks
Thank you for your Pull Request! We have run several checks on this pull request in order to make sure it's suitable for merging into this project. The results are listed in the following section.
Watched Files
This pull request modifies specific files that require careful review by the maintainers.
Files Matched
- npm-shrinkwrap.json
- package.json
Great job, one minor bug I can see is that the initial theme is still set from the code to |
Ahh, I see. I'll check that out. I have a question about the system theme actually. Are we still going to support the system theme if there is an element that only has two states as opposed to the 3 states we have? |
Not right now, supporting multiple themes requires a select drop down or multi value switch acting like a radio box. They make sense when we have color a11y but not now, the way the system theme is implemented it can serve a sane default to our light/dark switch. TL;DR: keep light/dark themes, repurpose system to provide default state to the switch |
I was going to say this as well. Set the default with system if provided, otherwise safe to keep it a binary switch. |
@chadstewart can you also add this change to the storybook as well? |
β¦m theme was selected, the system was on dark mode and the user clicked on the switch, the first click would not do anything. Added the theme switch component to Storybook.
Going to merge this now, but opening an issue re: storybook. |
* Removed theme buttons in theme button group and replaced with radix switch. * Updated code based on PR comments. Solved an issue where if the system theme was selected, the system was on dark mode and the user clicked on the switch, the first click would not do anything. Added the theme switch component to Storybook. 0805da7
## [0.46.0](v0.45.1...v0.46.0) (2022-04-11) ### π Features * Removed theme buttons and replaced them with radix switch ([#1417](#1417)) ([0805da7](0805da7))
π This PR is included in version 0.46.0 π The release is available on: Your semantic-release bot π¦π |
What type of PR is this? (check all applicable)
Description
This PR [adds] the [feature] of changing the theme by using a switch element instead of separate buttons.
Related Tickets & Documents
Closes #1402
Mobile & Desktop Screenshots/Recordings
Added tests?
Added to documentation?
[optional] Are there any post-deployment tasks we need to perform?
[optional] What gif best describes this PR or how it makes you feel?