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. |
| 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.
Is the message complete? Maybe "...Uploaded FILE"?
There was a problem hiding this comment.
Nice catch! Probably uploaded "file(s)" to also include the possibility of multiple non-submitted files.
There was a problem hiding this comment.
wow, not sure how that happened honestly. code goblins 😄
| [isVisible, modalRef, buttons] | ||
| ) | ||
|
|
||
| const onTabPressed = (shiftKey = false) => { |
There was a problem hiding this comment.
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.
reitermb
added
Deploy with CircleCI-a11y
There was a problem hiding this comment.
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? |
ADPennington
added
QASP Review
and removed
raft review
ADPennington
added
the
Deploy with CircleCI-qasp
There was a problem hiding this comment.
👍🏾 confirmed the following:
- changing selections as a
data analystorofa adminafter upload and clicking search triggers the warning modal - clicking
cancelin 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 searchin 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.
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. |
ADPennington
added
Ready to Merge
and removed
QASP Review
Gov A11y Review
Deploy with CircleCI-qasp
👍 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 |
andrew-jameson
deleted the
fix/992-notify-data-file-search-selection-changes
branch
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:
lfrohlichand/oradpenningtonconfirmed that ACs are met.Deliverable 2: Tested Code
CodeCov Reportcomment in PR)CodeCov Reportcomment in PR)Deliverable 3: Properly Styled Code
Deliverable 4: Accessible
iamjollyandttran-hubusing 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):