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

Feat/switch light theme #609

Merged
merged 8 commits into from
Dec 23, 2024
Merged

Feat/switch light theme #609

merged 8 commits into from
Dec 23, 2024

Conversation

gregmartDOTin
Copy link
Contributor

No description provided.

Copy link
Contributor

github-actions bot commented Dec 17, 2024

PR Preview Action v1.4.8
Preview removed because the pull request was closed.
2024-12-23 18:13 UTC

@@ -12,6 +17,29 @@ export type SwitchProps = Omit<
| 'touchRippleRef'
>;

export const Switch = (args: SwitchProps): JSX.Element => {
const CheckedIconSwitch = styled(MuiSwitch)(({ theme }) => ({
Copy link
Contributor

Choose a reason for hiding this comment

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

do we also want to fix this for legacy theme?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, yeah, good call. I'll fix it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

args: {
sx: { marginRight: '6px' },
inputProps: {
'aria-label': 'example',
Copy link
Contributor

Choose a reason for hiding this comment

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

Personally, I prefer the bare component with no label for the primary story, but if we're keeping FormControlLabel let's ditch the aria-label

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had a conversation with UX about this. The pattern in zeroheight expects an action label, so this would be the most common use case.

Copy link
Contributor

Choose a reason for hiding this comment

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

but the primary story isn't related to use case, it's the component itself... i think UX is misunderstanding that dev docs are different than their use cases.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That makes sense. I switched back to the Switch story being a bare Switch component and added the With Label story.

@LauRoxx LauRoxx linked an issue Dec 20, 2024 that may be closed by this pull request
@gregmartDOTin gregmartDOTin merged commit 47403fe into main Dec 23, 2024
8 checks passed
@gregmartDOTin gregmartDOTin deleted the feat/switch-light-theme branch December 23, 2024 18:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Create Switch Component
2 participants