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

Components: Improve Popover computations #11430

Closed
2 of 4 tasks
aduth opened this issue Nov 2, 2018 · 3 comments
Closed
2 of 4 tasks

Components: Improve Popover computations #11430

aduth opened this issue Nov 2, 2018 · 3 comments
Assignees
Labels
[Feature] UI Components Impacts or related to the UI component system [Type] Task Issues or PRs that have been broken down into an individual action to take

Comments

@aduth
Copy link
Member

aduth commented Nov 2, 2018

The following comments should be resolved or addressed:

  • 1.

What about:

Where are we canceling this timeout if the component is unmounted?

#6799 (comment)

Edit: Addressed in #8218


  • 2.

This is treating the symptom, not the cause, and the comment reads as such.

The real cause is a combination of:

  • When initially mounted, the popover doesn't have a Slot context in which to render. When this becomes available, it re-renders the content into the slot (an unmount and remount operation).
  • When initially mounted, the container has a style of visibility: hidden, so focusing would not select the container.

#6799 (comment)


  • 3.

Related to earlier comment, we shouldn't need to check this here. If the component causes Popover#focus to be called during its componentDidMount, we should be able to assume that the ref.current is assigned.

#6799 (comment)


  • 4.

It took me more than a minute to grasp why this condition is here. It seems obvious in retrospect, but a comment would have been nice.

#6799 (comment)

@aduth aduth added the [Feature] UI Components Impacts or related to the UI component system label Nov 2, 2018
@aduth aduth added the [Type] Task Issues or PRs that have been broken down into an individual action to take label Nov 2, 2018
@youknowriad
Copy link
Contributor

Closing this issue as the Popover component changed a lot, most of these are not relevant anymore.

@aduth
Copy link
Member Author

aduth commented Mar 11, 2020

I updated the original comment. Only the last is invalid. The second and third are still problems.

@aduth aduth reopened this Mar 11, 2020
@ciampo
Copy link
Contributor

ciampo commented Mar 28, 2022

Regarding points no. 2 and 3 from the issue's description, it looks like the main issue being discussed was around focus management (and in particular, moving the focus to the first tabbable element inside the popover).

It looks like thie feature is now achieved through the useDialog hook, which internally relies on the useFocusOnMount utility hook.

I believe we can close this issue (of course, @aduth, feel free to reply in case you think I missed something!)

@ciampo ciampo closed this as completed Mar 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] UI Components Impacts or related to the UI component system [Type] Task Issues or PRs that have been broken down into an individual action to take
Projects
None yet
Development

No branches or pull requests

3 participants