-
Notifications
You must be signed in to change notification settings - Fork 74
Select Valid ID Form #1074
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
base: new-main-page-UX
Are you sure you want to change the base?
Select Valid ID Form #1074
Conversation
dmols
left a comment
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.
Form works well, great work! I only spotted a few things that may need tweaking so that it's ready for merge:
- I'd probably change "Use Attribute" to "Restore Attribute" as it did remove it entirely, and that sounds a little more fitting. Also not entirely sold on the gray, but it probably needs a bit more contrast between the button and the form instructions background.
- Nitpick but is it possible to not have the divider for these, where it's the last one in the list? It looks kind of odd before the
ORdivider.
- It seems like when the issue is fixed, the form cannot grab what the error used to be. Is there a way we can make it so it retains this information? That way, if a user decides to change some IDs back, they can go back to this form to do so.
- We should also remove the description above the "Please select an ... " as other forms don't have any text above the form instructions (at least in the same box). You could potentially just change "Please select an attribute you want to add IDs to" to "Select an attribute below and click an element on the preview screen to link to" or something along those lines. There's probably a better way to condense that.
dmols
left a comment
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.
Changes look great! We probably just need a few more accessibility changes for it to be ready to merge.
- We need a widget on the form that has the role of alert, with inner html that changes based on what element is selected on the right, and which aria attribute we want the element to be. The aria alert would only trigger when we select the element on the preview screen.
- Instructions for the screen reader would be useful for when the user selects any of the form options (
aria-controls,aria-describedby). Ideally, it's an assertive aria-live since we want the user to know what to do on the preview screen once they select an option. - More so for keyboard users, but it'd be a good idea to shift focus to the preview screen as soon as a selection is made on the left. Once you select an item on the right, it should also shift focus the user's attention back to the form, so they can select another form option. This would be more like how we generally perceive the flow.
Let me know if you have any questions!
Summary
Adds the "Select Valid ID Form" along with the ability to click elements on the preview for interactions.
Key Changes
SelectValidIDForm.jsFixIssuesPage.jswhich uses both theactiveIssueand the currentactiveContentItemto saveClickableInfoinFixIssuesPage.jsto keep track of what is being clickedTempActiveContentItemto keep track of clicking and changes to DOMTesting
The form should behave as the following: