-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
docs(Toggles): add label examples to stories; remove a11yProps #6770
docs(Toggles): add label examples to stories; remove a11yProps #6770
Conversation
Deploy preview for carbon-elements ready! Built with commit 842d8be |
Deploy preview for carbon-components-react ready! Built with commit 842d8be https://deploy-preview-6770--carbon-components-react.netlify.app |
Deploy preview for carbon-elements ready! Built with commit 623e340 |
Deploy preview for carbon-components-react ready! Built without sensitive environment variables with commit 623e340 https://deploy-preview-6770--carbon-components-react.netlify.app |
Just noticed there is a difference in margin between the skeleton label and the non-skeleton label. Also noticed that there are no skeleton styles... If we don't have skeleton styles defined (not sure they are needed for a toggle?) perhaps we should just remove the examples and any styles associated with them |
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.
Looks good outside of the feedback from TJ above 👍
bump @dakahn when you have a sec 👀 |
…n/carbon into add-label-examples-to-toggle-story
@tw15egan sorry, coming back to this after -- too too long. I fixed the spacing issue by getting the skeleton code in line with the regular toggle code. So you think we should maybe scrap Toggle.Skeleton and all it's styles altogether? |
For skeleton I bet we could just remove it from the story until we add in the styles for it 👀 |
Yeah I'd just remove the skeleton stories until we implement it 😄 Everything else looks great! |
…n/carbon into add-label-examples-to-toggle-story
I think we just need to remove the |
@tw15egan oh duh, sorry |
bump @tw15egan I think it's missing 1 review for the auto-merge if you have a sec to approve 👀 |
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.
🎉
Noticed while triaging an issue that we didn't have labels on our Toggle examples. So I threw some on Toggle and ToggleSmall 👍
Also removed a11yProps and just added the label to the main props object.
Also removed the unneeded aria label since we have explicit labeling with the
<label>
element 🏄