Skip to content
This repository has been archived by the owner on May 25, 2023. It is now read-only.

feat: Removed theme buttons and replaced them with radix switch #1417

Merged
merged 2 commits into from
Apr 11, 2022

Conversation

chadstewart
Copy link
Contributor

@chadstewart chadstewart commented Apr 7, 2022

What type of PR is this? (check all applicable)

  • πŸ• Feature
  • πŸ› Bug Fix
  • πŸ“ Documentation Update
  • 🎨 Style
  • πŸ§‘β€πŸ’» Code Refactor
  • πŸ”₯ Performance Improvements
  • βœ… Test
  • πŸ€– Build
  • πŸ” CI
  • πŸ“¦ Chore (Release)
  • ⏩ Revert

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

Screenshot from 2022-04-06 20-54-58
Screenshot from 2022-04-06 20-55-03

Added tests?

  • πŸ‘ yes
  • πŸ™… no, because they aren't needed
  • πŸ™‹ no, because I need help

Added to documentation?

  • πŸ“œ README.md
  • πŸ““ docs.opensauced.pizza
  • πŸ• dev.to/opensauced
  • πŸ“• storybook
  • πŸ™… no documentation needed

[optional] Are there any post-deployment tasks we need to perform?

[optional] What gif best describes this PR or how it makes you feel?

Copy link
Contributor

@github-actions github-actions bot left a 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

@0-vortex
Copy link
Contributor

0-vortex commented Apr 7, 2022

Great job, one minor bug I can see is that the initial theme is still set from the code to light/dark/system and in the case of system it will load the theme switch as light when the theme is dark - switching it to dark will not have any effects

@chadstewart
Copy link
Contributor Author

chadstewart commented Apr 7, 2022

Great job, one minor bug I can see is that the initial theme is still set from the code to light/dark/system and in the case of system it will load the theme switch as light when the theme is dark - switching it to dark will not have any effects

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?

@0-vortex
Copy link
Contributor

0-vortex commented Apr 7, 2022

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

@bdougie
Copy link
Member

bdougie commented Apr 7, 2022

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.

@bdougie
Copy link
Member

bdougie commented Apr 7, 2022

@chadstewart can you also add this change to the storybook as well?

@chadstewart
Copy link
Contributor Author

Completely forgot about that. Sorry about that. I'll add it to Storybook when I go to resolve the bug in this PR.

Thanks for the feedback @bdougie and @0-vortex!

…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.
@bdougie
Copy link
Member

bdougie commented Apr 11, 2022

Going to merge this now, but opening an issue re: storybook.

@bdougie bdougie merged commit 0805da7 into main Apr 11, 2022
@bdougie bdougie deleted the feature/dark_mode_buttons_to_radix_switch branch April 11, 2022 19:24
github-actions bot pushed a commit that referenced this pull request Apr 11, 2022
* 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
github-actions bot pushed a commit that referenced this pull request Apr 11, 2022
## [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))
@github-actions
Copy link
Contributor

πŸŽ‰ This PR is included in version 0.46.0 πŸŽ‰

The release is available on:

Your semantic-release bot πŸ“¦πŸš€

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature: Replace dark mode buttons with a radix switch.
3 participants