Add read-only Switch #5902
Add read-only Switch #5902
Conversation
🦋 Changeset detectedLatest commit: 567fb33 The changes in this PR will be included in the next version bump. This PR includes changesets to release 5 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
| border: 0; | ||
| } | ||
|
|
||
| .salt-density-high .saltSwitch-readOnly .saltSwitch-thumb { |
There was a problem hiding this comment.
Right above sets border to zero for some reason
https://github.com/jpmorganchase/salt-ds/pull/5902/changes/BASE..4a562019d7b63cb4f01e646299925842aa7ae743#diff-5b3ab8c0000859aab2212f3251e991de737f6728c8cc74a2d002e54d14920025L115
There was a problem hiding this comment.
I assume it's to do with sizing, could you check with the designers if what is there meets the expected outcome please
There was a problem hiding this comment.
Figma doesn't have a stroke on the switch thumb in all the densities. Read-only states look correct though.
|
|
||
| ## Best Practice | ||
|
|
||
| Ideally we shouln't use readOnly |
There was a problem hiding this comment.
Do we need to talk more about why we shouldn't? We included a note in the example section, wondering if we should move it here? @karlgoldstraw
There was a problem hiding this comment.
@wemmyo I was walking Karl through how to edit the docs. I believe he might add more information to this but I'll take a look as well. Also, I did make a slight change to the read only note and tried to align it with what we are using elsewhere (Checkbox for example). Let me know what you think.
There was a problem hiding this comment.
Yeah sounds good thanks!
This reverts commit 8c267b7.
| A read-only switch allows the user to select and copy its text, but does not allow them to change its state. You can set a switch as read-only via the prop `readOnly`. | ||
|
|
||
| **Note:** We recommend avoiding read-only switches due to the lack of adequate `aria-readonly` support and conflicting user expectations. If you must use a read-only switch, you can add visual or non-visual labels to make its state clear. | ||
|
|
There was a problem hiding this comment.
Copying and pasting a label does not seem to be of that much value but that's how we are selling read-only.
What is more valuable is a focusable control being announced by the screen reader, whilst a disabled control cannot be focused and so will not be seen by the screenreader.
- Lack of adequate support for aria-readonly, we are not clear enough hereon the impact, I guess you browser and screenreader combination.
There was a problem hiding this comment.
I believe generally we're not trying to sell the readonly switch because of it's limitations including limited support for screen readers, but it's something we have just to be consistent with similar components. Guidance here is similar to what we have for checkbox but @karlgoldstraw and @jake-costa can help expand on that
| Unlike disabled switches, read-only switches are focusable and shown in the tab order when using a keyboard, allowing users to interact with its text. Use a read-only switch when its text contains information that is valuable to the user. | ||
|
|
||
| <LivePreview componentName="switch" exampleName="Readonly" /> | ||
|
|
There was a problem hiding this comment.
No guidance currently on the accessibility tab.
Feels like accessibility info is on this page instead ?
There was a problem hiding this comment.
From @jake-costa's comment earlier @karlgoldstraw might be adding more here but we thought the note in the examples section might be beneficial. We can move it to the accessibility tab if that's better
There was a problem hiding this comment.
@wemmyo @mark-tate In addition, we have this kind of language on other read only examples (checkbox, radio, etc.)
|
|
||
| it("THEN should not toggle when pressing the Space key", () => { | ||
| cy.mount(<Switch readOnly />); | ||
| cy.findByRole("switch").should("not.be.checked"); |
There was a problem hiding this comment.
no test of focus but it's something we call out as a differentiator between disabled?
There was a problem hiding this comment.
read-only attribute as well ?
There was a problem hiding this comment.
Good point thanks, I've included them in the tests
Closes #3840