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

Update explainer with recent resolutions #538

Merged
merged 2 commits into from
Jun 4, 2022

Conversation

Copy link
Member

@gregwhitworth gregwhitworth left a comment

Choose a reason for hiding this comment

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

Left some comments that are worth addressing but don't need to block this PR

research/src/pages/popup/popup.research.explainer.mdx Outdated Show resolved Hide resolved
research/src/pages/popup/popup.research.explainer.mdx Outdated Show resolved Hide resolved
research/src/pages/popup/popup.research.explainer.mdx Outdated Show resolved Hide resolved
- When a popup is hidden, focus is set back to the **previously focused element**, if it is non-`null`, in the following cases:
1. Light dismiss via close signal (e.g. Escape key pressed).
2. Hide popup from Javascript via `hidePopup()`.
3. Hide popup via a **popup-contained** triggering element with `hidepopup=popup_id` or `togglepopup=popup=id`. The triggering element must be popup-contained, otherwise the keyboard or mouse activation of the triggering element should have already moved focus to that element.
Copy link
Member

Choose a reason for hiding this comment

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

Again not blocking this PR but we should have some examples of these solutions.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I added another sentence, but I'm open to specific suggestions for examples here.

@gregwhitworth
Copy link
Member

@scottaohara @dandclark can you all give this a quick review. @mfreed7 if this isn't reviewed by the upcoming Thursday go ahead and land it as they can always file new issues if they find it.

Copy link
Collaborator

@scottaohara scottaohara left a comment

Choose a reason for hiding this comment

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

just a few things which also do not necessarily need to be merged now to land this PR. thank you again for the work on updating this!

research/src/pages/popup/popup.research.explainer.mdx Outdated Show resolved Hide resolved
research/src/pages/popup/popup.research.explainer.mdx Outdated Show resolved Hide resolved
research/src/pages/popup/popup.research.explainer.mdx Outdated Show resolved Hide resolved
Copy link
Collaborator

@dandclark dandclark left a comment

Choose a reason for hiding this comment

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

Thanks for making these updates!

research/src/pages/popup/popup.research.explainer.mdx Outdated Show resolved Hide resolved
research/src/pages/popup/popup.research.explainer.mdx Outdated Show resolved Hide resolved
@mfreed7
Copy link
Collaborator Author

mfreed7 commented Jun 1, 2022

Thanks for all of the reviews! I've addressed all of the comments, I think. LMK if you see anything else.

@mfreed7
Copy link
Collaborator Author

mfreed7 commented Jun 3, 2022

Any other comments? Mind if we land this?

@andrico1234 andrico1234 merged commit 960b3de into openui:main Jun 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment