M3-4582 Change: Use a shared component for view-only permissions tables.#7169
M3-4582 Change: Use a shared component for view-only permissions tables.#7169Jskobos merged 3 commits intolinode:developfrom Jskobos:M3-4582-read-only-permissions
Conversation
For OBJ access tokens and API access tokens, instead of showing a disabled version of the scopes form (with a bunch of radio buttons), if a user is in view-only mode instead use check marks for selected permissions. I opted to go with a shared (new) component for this, though it isn't clear if it's worth it in this case. Use check marks instead of disabled/greyed table cells Refactor out an AccessCell component Fix aria label
|
|
||
| return (closeMenu: Function): Action[] => { | ||
| const actions = !isThirdPartyAccessToken | ||
| return !isThirdPartyAccessToken |
There was a problem hiding this comment.
These are linter fixes
johnwcallahan
left a comment
johnwcallahan
left a comment
There was a problem hiding this comment.
UI looks good, one bug in the PAT drawer.
Not sure about the accessibility here. Maybe the span containing the SVG needs something like:
role="checkbox"
aria-checked="true"
aria-disabled="true"
| value="0" | ||
| <AccessCell | ||
| active={scopeTup[1] === 0} | ||
| scope="2" |
There was a problem hiding this comment.
Think this should be "0"... currently there's some incorrect behavior when you click "None" for a single scope.
I'm not sure about accessibility either, I feel weird telling people it's a checkbox when it isn't. I added an aria-label to the span instead, let me know what you think. @tiffwong please weigh in as well. |
johnwcallahan
left a comment
johnwcallahan
left a comment
There was a problem hiding this comment.
The ARIA label makes sense to me.
| return ( | ||
| <span | ||
| className={classes.checkIcon} | ||
| aria-label={`This token has ${scope} access for ${scopeDisplay}`} |
There was a problem hiding this comment.
Currently, the aria-label isn't accessible so this never gets read but can easily be fixed by adding tabIndex={0}
tiffwong
left a comment
tiffwong
left a comment
There was a problem hiding this comment.
This looks great! The check icons are a great visual touch to prevent users from thinking they can edit the permissions 💯
The only comment I have is to add a tabIndex so that users can access the aria-label and I agree with not labeling it as a checkbox since that could confuse the user into thinking they can edit it (which defeats the whole purpose of this)
3 participants
Description
For OBJ access tokens and API access tokens, instead of showing a disabled version of the scopes form (with a bunch of radio buttons), if a user is in view-only mode instead use check marks for selected permissions. I opted to go with a shared (new) component for this, though it isn't clear if it's worth it in this case.
@ndukatz there are a few existing e2e tests that used data-qa attributes that are lost using this approach (nothing in Cypress though). Please let me know if I broke anything you're working on.
@tiffwong making this change meant that the aria stuff needed to be dynamic. The results are a bit different but I think they're ok. Please let me know.