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

Added switch component (#1299) #1328

Merged
merged 8 commits into from
Jul 5, 2024
Merged

Conversation

CalebLuster
Copy link
Collaborator

Copy link

cloudflare-workers-and-pages bot commented Jul 2, 2024

Deploying design-system with  Cloudflare Pages  Cloudflare Pages

Latest commit: 5773b44
Status: ✅  Deploy successful!
Preview URL: https://be9b2d21.design-system.pages.dev
Branch Preview URL: https://issue-1299-create-new-switch.design-system.pages.dev

View logs

}

&__value {
font-size: 16px;
Copy link
Member

@Jura Jura Jul 2, 2024

Choose a reason for hiding this comment

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

Please use relative units when possible and makes sense. This should be 1rem;

{showLabel && <div className="switch__label">{label}</div>}
<div className='switch__wrapper'>
<div className="switch__track" tabIndex={0}>
{showIcon && (
Copy link
Member

Choose a reason for hiding this comment

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

To avoid using css filters (and hardcode color value) you might want to replace element with

, which has respective .svg applied as a mask.
Check Language switcher component's icons for the example of implementation

@Jura
Copy link
Member

Jura commented Jul 5, 2024

@CalebLuster , as discussed, since we are using button element as a toggle (not default behavior) we shall use associated aria- attributes to explicitly declare that. Check out https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Attributes/aria-pressed for details, but essentially you should add aria-pressed attribute to the button element with true/false values reflecting current state.

Another accessibility issue we have to address is that element must have a label all the time. Regardless its visibility, which will be governed by that control, but markup should include label all the time. To achieve that use persistent aria-label attribute for the button.

…ted with NVDA, everything seems to be working well
`;

useEffect(() => {
console.log('toggled', toggled);
Copy link
Member

Choose a reason for hiding this comment

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

@CalebLuster , please remove console.log call, it shouldn't be a part of production code

@Jura
Copy link
Member

Jura commented Jul 5, 2024

@CalebLuster , changes in https://github.com/undp/design-system/pull/1328/files#diff-8c0b50e63802ff6a0da7dc6063f43563c1ae8b81f2d56d3bc34658edd34927e4 shouldn't be mixed with this ticket. We will leave it as is for the time being as it is not a big deal, but for the future - we should keep changes pertinent to specific ticket within relevant PR. THis one was marked as fixing #1299 , so other changes shouldn't be included as a matter of practice. No action required.

@Jura Jura merged commit 763fbdd into develop Jul 5, 2024
5 checks passed
@Jura Jura deleted the issue-1299-create-new-switch-component branch December 14, 2024 14:21
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.

2 participants