-
Notifications
You must be signed in to change notification settings - Fork 31
[FE-112] Boundary Enterprise a11y audit- User details form #2977
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
Open
venusang
wants to merge
5
commits into
main
Choose a base branch
from
fe-112-boundary-admin-a11y-user-deets
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+11
−3
Open
Changes from all commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
ed04192
refactor: 💡 add edit-user-details translations
venusang f7082ea
refactor: 💡 change inputs to readonly and update submit button
venusang 334e7d6
refactor: 💡 move translation to resources for user
venusang d86805c
refactor: 💡 removed aria-label from button set
venusang 2ec32be
fix: 🐛 Add aria-describedby to Edit user details button
venusang File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
I'm not sure I understand why we linked the ID of the form itself, what text are we trying to descriptively add to this by using
aria-describedby
? It also seems weird to use a translated text as the ID when the element itself should have the translated textThere 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.
I pinged you in the JIRA ticket comment but adding reference here for good measure.
(taking a screenshot so it has more context versus copy paste):
https://hashicorp.atlassian.net/browse/FE-112?focusedCommentId=828913
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.
Going off @MelSumner thoughts in the JIRA ticket, I set the form ID value to be
user-details
versususers
since to me it seems to make better sense that the form is specific to theuser-details
.Users
did not seem to make sense to me but maybe I am misunderstanding the purpose here.@ZedLi from what I am getting here, it sounds like the form ID value should be semantic versus the ember generated ID. Maybe Melanie could provide some clarity here for both of us. I'm not all that familiar with
aria-describedby
prior to working on this.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.
Right, I think all those choices make sense but it sounds like we just need to pick one choice? It seems we could leave the button as
Edit form
if we're adding one of the aria attributes.With regards to the current implementation with 3, I was specifically pointing out the fact that it's currently pointing to the form ID instead of something with descriptive text (which melanie suggests as the form title):
Though that looks like it has some extra text which would pulled in as part of the helper doc link text.
Uh oh!
There was an error while loading. Please reload this page.
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.
you wouldn't need to translate the element's
id
attribute value.Uh oh!
There was an error while loading. Please reload this page.
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.
Assuming this is what we are talking about:
How would we do this with
<Hds::Button />
?https://github.com/hashicorp/design-system/blob/main/packages/components/src/components/hds/button/index.hbs#L42-L44
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.
aria-description would be an attribute on the actual element itself, in this case the button in your example. We wouldn't be passing in a separate element but instead be passing the attribute to the button itself.
i.e.
My only question is if
aria-description
is OK to use? The docs mention:@MelSumner Do you happen to know if this is something that is just a formality and we're OK to use or we should avoid for now?
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.
fwiw, we can actually pass in
aria-description
into<Hds::Button />
(see ...attributes). I'll wait for @MelSumner to verify whether the code example that @ZedLi provided is acceptable. But I could use some help on what the description should be if we want to go in this direction.