update toggle component to work with app dashboard#112
Conversation
Osmose
left a comment
Osmose
left a comment
There was a problem hiding this comment.
Looks good, nice work! A few comments, but nothing blocking.
One lingering question: Is defaulting to blank instead of the original text okay? It'd remove the text from any toggles we're using once they upgrade, but you know more about the toggles on dotcom and whether they should have text or not than I do.
| height: var(--space-3); | ||
| width: var(--space-5); | ||
| font-weight: bold; | ||
| font-weight: 600; |
There was a problem hiding this comment.
bold maps to 700, not sure if that matters here.
There was a problem hiding this comment.
yeah, this is supposed to be semi-bold
lib/toggle.js
Outdated
| `; | ||
|
|
||
| const Shuttle = (props) => ( | ||
| const nbsp = ' '; |
There was a problem hiding this comment.
Maybe blank instead since this isn't a true  ?
There was a problem hiding this comment.
oh hm that was supposed to be a real nbsp, i'll fix that
| }; | ||
| Toggle.defaultProps = { | ||
| disabled: false, | ||
| trueLabel: '', |
There was a problem hiding this comment.
Should we have something default here?
Other lgtm
There was a problem hiding this comment.
I think it makes sense for the default labels to be blank (as they are on iOS), but we can add labels if we want them
sheridanvk
left a comment
sheridanvk
left a comment
There was a problem hiding this comment.
looks good, just one suggestion added!
lib/toggle.js
Outdated
| Toggle.propTypes = { | ||
| value: PropTypes.any.isRequired, | ||
| onChange: PropTypes.func.isRequired, | ||
| label: PropTypes.string.isRequired, |
There was a problem hiding this comment.
all these label names are a bit confusing now. I think this label is only used by aria-label now, right - shall we just call the parameter by the same name to make it clear (or input-aria-label)?
It is also a bit odd for the example that's provided in the story, because it separately has a "Notifications Settings" label, so I think we should really be passing in aria-labelledby for that example, with the ID of that label (per https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/ARIA_Techniques/Using_the_aria-label_attribute). We could leave that for a future PR though; perhaps a general accessibility sweep is needed.
sheridanvk
left a comment
sheridanvk
left a comment
There was a problem hiding this comment.
this is great, thanks for the tweak!
modernserf
deleted the
modernserf/ch16429/update-toggle-shared-component-for-app-dashboard
branch
|
🚀 PR was released in |
4 participants
https://app.clubhouse.io/glitch/story/16429/update-toggle-shared-component-for-app-dashboard-usage
disabledstate