-
Notifications
You must be signed in to change notification settings - Fork 3
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
Fix/992 notify data file search selection changes #2209
Fix/992 notify data file search selection changes #2209
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## develop #2209 +/- ##
===========================================
- Coverage 93.87% 93.80% -0.07%
===========================================
Files 53 91 +38
Lines 1713 2485 +772
Branches 218 337 +119
===========================================
+ Hits 1608 2331 +723
- Misses 85 110 +25
- Partials 20 44 +24
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
Negative case #1:
Negative case #2:
cc @ADPennington @reitermb Can we talk about this behavior? I feel like the error alert should force the user to reclick search. |
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.
LGTM! Tested locally
handleCancel={() => setIsToggled(false)} | ||
/> | ||
)} | ||
<Modal | ||
title="Files Not Submitted" | ||
message="Your files have not been submitted. Searching without submitting will discard your changes and remove any uploaded" |
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.
Is the message complete? Maybe "...Uploaded FILE"?
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.
Nice catch! Probably uploaded "file(s)" to also include the possibility of multiple non-submitted files.
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.
wow, not sure how that happened honestly. code goblins 😄
[isVisible, modalRef, buttons] | ||
) | ||
|
||
const onTabPressed = (shiftKey = false) => { |
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.
along with the focus()
in the useEffect
above, this is what "traps" the keyboard focus into the modal. the user should only be able to tab through (forward or backward by adding a shiftKey
modifier`) buttons within the modal's list.
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.
Looking good on focus trapping and visual z-index issues!
One last thing it still needs is an initial focus set to the modal's H1 though.
Currently the screenreader behavior results in a lack of context as to what the modal buttons are referring to.
e.g.
Modal opens
Screenreader says "Cancel"
The desired behavior here would be for focus to land on that H1 "Files Not Submitted" heading so that the behavior is:
Modal Opens
Screenreader says "Files Not Submitted"
User can then tab to buttons and have context for what they refer to
If it's not scope creeping this ticket it would be great to also give the paragraph text an ID that can then be referenced in an aria-describeby property on the modal's h1.
This would result in the screenreader user also being read the paragraph text, not just the heading text and enhance the overall clarity of the modal. That said, we can make this a follow-on issue if this adds scope (noting that this could also be an enhancement to our idle timer modal).
@reitermb thanks! i feel like we're getting close, pushed up both the requested changes. i tested the header focus change and that seems to work, but i don't have a screen reader set up to test the also, thinking ahead based on the convo in tabletop, i think it'd be helpful to discuss more about these a11y rules in general so we can make sure we're designing these reusable components appropriately and planning that into the initial scope of the work. |
@jtimpe @reitermb @ttran-hub is this ready for my review? |
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.
👍🏾 confirmed the following:
- changing selections as a
data analyst
orofa admin
after upload and clicking search triggers the warning modal - clicking
cancel
in the modal returns the user to the same changed selections that do not match the original selections reflected in the header description ✔️ - clicking
discard and search
in the modal, discards the uploaded files and shows the user to the results of the new search. ✔️ - if i change selections without clicking search and submit, the file submitted will have metadata that matches the header (the original selections ✔️
this is qasp- approved pending gov a11y review
cc: @ttran-hub @reitermb
@reitermb @ttran-hub this PR is in the qasp environment, which is only accessibly using non-acf credentials for sign-in. |
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.
Working great! That should do it for everything @ttran-hub and I discussed, including the describeby stretch goal. 💯
Video of it in action:
Modal pops up
Focus reads out heading
Paragraph text gets read automatically after heading
Maintains existing focus trap to contain users to the modal
chrome_5HxmtE6VY4.mp4
cc: @ADPennington
thanks @reitermb and @ttran-hub! @andrew-jameson this is ready to merge. |
👍 Always happy to talk a11y—is there a first step kind of sync you think would be helpful there? Re: Screenreaders, Thomas generally tests with NVDA—a popular screenreader for Windows. My main testing is a combination of that and Voiceover (for MacOS/iOS/iPadOS) with occasional Talkback (Android) testing thrown in just revisit mobile occasionally. In theory Voiceover is going to behave most consistently paired with Safari and NVDA paired with Firefox—but in practice we get consistent behavior cross-browser and cross-platform. Pending further syncing I also have a hack.md with some broader commentary on a11y tools that might be useful: https://hackmd.io/Z5t247a7St2tZzLS35JBkw?view |
Summary of Changes
Pull request closes #992
Change the fiscal year header on the reporting page to update without the search button being clicked (related finding mentioned in ticket details)How to Test
1. Notify the user when they are about to change their search params without having submitted their attached file.
2. Change the fiscal year header on the reporting page to update without the search button being clicked.Open http://localhost:3000/ and sign in.Approve your user and assign a role. In the frontend, navigate to the "Data Files" tab. Apply some search filters and click "search".Change the search filter values. The report header in the middle of the page should "live update" without the need for clicking the search button.This functionality was removed in favor of a more accessibility-friendly page flow. The user is now required to actually submit the search input form in order to both refresh the search screen and set the global state values for data file submission.
Deliverables
More details on how deliverables herein are assessed included here.
Deliverable 1: Accepted Features
Checklist of ACs:
lfrohlich
and/oradpennington
confirmed that ACs are met.Deliverable 2: Tested Code
CodeCov Report
comment in PR)CodeCov Report
comment in PR)Deliverable 3: Properly Styled Code
Deliverable 4: Accessible
iamjolly
andttran-hub
using Accessibility Insights reveal any errors introduced in this PR?Deliverable 5: Deployed
Deliverable 6: Documented
Deliverable 7: Secure
Deliverable 8: User Research
Research product(s) clearly articulate(s):