-
Notifications
You must be signed in to change notification settings - Fork 152
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: Show user an "Assessment saved" dialog after they save the assessment #5770
fix: Show user an "Assessment saved" dialog after they save the assessment #5770
Conversation
export const SaveAssessmentButton = NamedFC<SaveAssessmentButtonProps>( | ||
'SaveAssessmentButton', | ||
props => { | ||
const [hideDialog, { toggle: toggleHideDialog }] = useBoolean(true); |
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 haven't seen these fluent-UI hook utility functions before! I understand it now but had to trace it at first. I think(?) this is the first usage of fluent-ui/react-hooks so we might consider a quick ping to interested web-folks to validate that there are no concerns. I'm good either way 🙂
At our lowest supported width (1280px wide by 400% zoom), the "Don't show again" text reflows kind of awkwardly: Ideally:
|
The NVDA+Edge behavior for this new dialog feels a bit awkward because the announcement of the dialog gets talked over by the browser's announcement of the download completing; the resulting readout feels to me like it's implying that our dialog controls (the "don't show again" and "got it" controls) are elements of the browser's file download UX, not a page-specific dialog. @RobGallo, would you mind evaluating the new dialog for usability with your JAWS setup? |
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.
Left a few comments on usability/UX of the new dialog, will take a look over the code once we've nailed down the user experience
I received @RobGallo's evaluation (a few days ago, I was focused elsewhere😅) in which he agrees that the text is confusing, but comprehendible given the constraints that we have. Another I option I explored was the downloads.onChanged event. The component would listen for when the file downloaded and then trigger the modal. When testing with NDVA, the dialog is announced. The implementation could something like: React.useEffect(() => {
browser.downloads.onChanged.addListener(handleDownloadedAssessment);
return () => {
browser.downloads.onChanged.removeListener(handleDownloadedAssessment);
};
}, []);
function handleDownloadedAssessment(event) {
if (event.state && event.state.current === 'complete') {
props.deps.detailsViewActionMessageCreator.saveAssessment(event);
if (props.userConfigurationStoreData.showSaveAssessmentDialog) {
showDialog();
}
}
} However, this API is limited to chromium and requires manifest.json "downloads" permission. It's not clear to me if either of those are dealbreakers (or if we prefer not to use an event listener). @dbjorge what you think about using the |
...w-overlay/settings-panel/settings/save-assessment-dialog/save-assessment-dialog-settings.tsx
Show resolved
Hide resolved
...-view-overlay/settings-panel/settings/save-assessment-dialog/save-assessment-dialog.test.tsx
Outdated
Show resolved
Hide resolved
...-view-overlay/settings-panel/settings/save-assessment-dialog/save-assessment-dialog.test.tsx
Outdated
Show resolved
Hide resolved
...tests/unit/tests/background/global-action-creators/user-configuration-action-creator.test.ts
Outdated
Show resolved
Hide resolved
src/tests/unit/tests/DetailsView/components/should-show-report-export-button.test.ts
Outdated
Show resolved
Hide resolved
src/tests/unit/tests/DetailsView/components/save-assessment-button.test.tsx
Outdated
Show resolved
Hide resolved
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.
Thanks for investigating the AT behavior with Rob, Katy! Per our discussion offline, I don't think the issue with the AT reading is enough to justify a new permission on the app, even if it does read a bit awkwardly.
Reviewed the code, left a few suggestions inline.
Update save-assessment-button.test.tsx
ae0b07d
to
b1b7b37
Compare
028be3d
to
0387022
Compare
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!
Details
This pull request will add an Assessment saved dialog after the users saves an assessment. In the dialog, the user can click "Don't show again" to dismiss the dialog. Like the tab stops dialog, they can reset this value from the settings panel.
The image below shows the "Assessment save dialog" with the option to "Don't show [dialog] again" and "Got it" button to dismiss the dialog:
The image below shows the settings panel, where the user can toggle to enable the save assessment dialog:
When building this feature, I followed the patterns with the tab stops dialog. I found one small improvement with the tab stops dialog that I made with saved assessment: use the
Stack
component to visually line-up the Do not show again and Got it buttons in the dialog footer. The table below shows screenshots of the current tab stops dialog and proposed:Motivation
Addresses #5608
Context
Pull request checklist
yarn fastpass
yarn test
)<rootDir>/test-results/unit/coverage
fix:
,chore:
,feat(feature-name):
,refactor:
). SeeCONTRIBUTING.md
.