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

Refactor FeedbackWidget with Chakra-UI #11906

Merged
merged 7 commits into from
Jan 31, 2024
Merged

Refactor FeedbackWidget with Chakra-UI #11906

merged 7 commits into from
Jan 31, 2024

Conversation

wackerow
Copy link
Member

Description

Updates FeedbackWidget component, refactored using Chakra-UI AlertDialog

  • Removes need for focus-trap-react package previously being used
  • Removes need for useKeyPress hook previously being used
  • Removed need for useOnClickOutside hook previously being used

Copy link

netlify bot commented Jan 11, 2024

Deploy Preview for ethereumorg ready!

Name Link
🔨 Latest commit 332b8ef
🔍 Latest deploy log https://app.netlify.com/sites/ethereumorg/deploys/65b1cbfc63007e0008250da1
😎 Deploy Preview https://deploy-preview-11906--ethereumorg.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.

@TylerAPfledderer
Copy link
Contributor

@wackerow should close #10997 in favor of this, correct?

@wackerow
Copy link
Member Author

@TylerAPfledderer Curious your thoughts if you see anything from that PR we should incorporate here, but I think its a bit cleaner utilizing the AlertDialog here

@corwintines corwintines self-assigned this Jan 24, 2024
@TylerAPfledderer
Copy link
Contributor

TylerAPfledderer commented Jan 25, 2024

@TylerAPfledderer Curious your thoughts if you see anything from that PR we should incorporate here, but I think its a bit cleaner utilizing the AlertDialog here

@wackerow I intended to go ahead and add a story for it.

I would note that AlertDialog adds role='alertdialog' to the modal. By MDN definition for screen readers:

The alertdialog role is used to notify users of urgent information that demands the user's immediate attention. Including role="alertdialog" on the element containing the dialog helps assistive technology identify the content as being grouped and separated from the rest of the page content. Examples include error messages that require confirmation and other action confirmation prompts.

When testing with NVDA, it doesn't seem to have that kind of effect here. However, unlike the current version, a screen reader might readout everything in the modal immediately.

Here are the following set of prompts NVDA gives me as soon as I select the button to open the modal:

Is this page helpful? dialog

clickable Close button

Is this page helpful?

button Close

Is this page helpful?

button Yes
button No

Would be better to get proper user feedback, but this looks to be an improvement.

@wackerow
Copy link
Member Author

@konopkja Btw, this PR fixes the bug you noted about "Closed feedback widget" events being triggered inappropriately.

@corwintines corwintines merged commit 445b8ec into dev Jan 31, 2024
9 of 10 checks passed
@corwintines corwintines deleted the feedback-widget branch January 31, 2024 20:13
This was referenced Jan 31, 2024
This was referenced Feb 14, 2024
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