Skip to content
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

DEV-1177: finish up feedback form fixes #88

Merged
merged 3 commits into from
Jul 2, 2024
Merged

Conversation

carylwyatt
Copy link
Member

@carylwyatt carylwyatt commented Jul 1, 2024

The last couple fixes for the feedback form and feedback modal

issue 1727230: identify input purpose missing for name and email

All three feedback forms were missing "autocomplete" HTML attributes on name and email fields, so I added them. See FeedbackFormBasic/index.svelte, FeedbackFormCatalog/index.svelte, and FeedbackFormContent/index.svelte.

To test: on dev-3, open the feedback form using the GET HELP dropdown in the navbar. Use dev tools to inspect the name input element and see the autocomplete attribute set to "name". The email input element has the autocomplete attribute set to "email".

issue 1727229: when modal is closed, focus isn't returned to trigger

When any modal is closed, in order to remain accessible, the browser/user's focus should be returned to the element that triggered the opening of the modal. In this case, the triggering element is in the navbar dropdown under GET HELP. The feedback form opens when a user clicks "Ask a Question" or "Report a Problem." I originally fixed this so that the focus would return to either of those links, but the fix included not-best-practices javascript in order to reopen the dropdown that would normally close on-click. Deque says it's fine to return the focus to the dropdown link element (GET HELP) instead of the actual triggering element, so I undid all that junk and did something much simpler: added a boolean prop focusHelpOnClose to the FeedbackFormModal component to pass down to the Modal component. If that value is true, the focus is returned to GET HELP when the modal is closed.

To test: on dev-3 (or locally if you want to pull down the branch), use your keyboard to navigate through the navbar to "GET HELP." Hit enter to toggle the dropdown, then select either of the feedback forms. The dropdown should close, a modal should open. Now hit enter again to close the modal (the close button should be auto-focused), and see that the focus ring returns to the GELP HELP link. You can also check that the login modal has similar behavior.

Copy link

netlify bot commented Jul 1, 2024

Deploy Preview for hathitrust-firebird-common ready!

Name Link
🔨 Latest commit d6ff376
🔍 Latest deploy log https://app.netlify.com/sites/hathitrust-firebird-common/deploys/668310ec3939210008e353e8
😎 Deploy Preview https://deploy-preview-88--hathitrust-firebird-common.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@carylwyatt carylwyatt requested review from mwarin July 2, 2024 15:06
Copy link

@mwarin mwarin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could see the autocomplete attributes set correctly, as promised. The values I enter in the form modal are remembered until I submit the form or navigate to another page. Are they supposed to pre-populate if I'm logged in? If so, did not check that.

Focus is returned to GET HELP when I close or submit the form modal, as promised. This worked well for me whether navigating with mouse, keyboard, or screen reader.

I have 2 bits of feedback, neither of which block approval:

1: As a user relying on keyboard/screen-reader, I think it would be neat if the form modal would close on ESC, rather than having to tab back to the beginning of the form to close it. This might be accessibility-heresy, so take that as the personal preference it is.

2: When I've submitted my question/feedback, the form modal goes into a "submitted, start over?" state. Fine. But if I go to submit another question/feedback the modal pops up in this "submitted, start over?" state again. It would make more sense if the form reset if I've dismissed it once.

That said, looks good, APPROVE.

@carylwyatt
Copy link
Member Author

  1. ESC to close the modal should be default behavior for any modal. I'm surprised Deque didn't catch that! I'll open a new issue.
  2. This is related to another known "state" issue in the feedback forms: if you submit a form with errors in it, you get some messaging about incorrect form fields. If you close the form and open it again, you would expect to see a blank form, but instead, you get a form with "incorrect field" error messages. Resetting the form when the modal is closed seems like a good idea to me, too. Another issue to open!

Thanks for the review! 🙌

@carylwyatt carylwyatt merged commit fc19c80 into main Jul 2, 2024
10 checks passed
@carylwyatt
Copy link
Member Author

@giramesh @angelinanz DEV-1177 fixes are now staged on test.www.hathitrust.org and ready for final review!

@carylwyatt carylwyatt deleted the DEV-1177-feedback-form branch July 2, 2024 20:30
@giramesh
Copy link

giramesh commented Jul 9, 2024

Issues: 1727230 | 1727229 -tested the fixes and I can confirm that these are solved!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants