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

[core] feat(Overlay): add shouldReturnFocusOnClose prop #4904

Merged
merged 1 commit into from Sep 14, 2021

Conversation

adidahiya
Copy link
Contributor

@adidahiya adidahiya commented Sep 14, 2021

Checklist

  • Includes tests
  • Update documentation

Changes proposed in this pull request:

Building on #4422 and #4620, this PR moves the shouldReturnFocusOnClose functionality to the Overlay component, thereby enabling it by default for Popover/Popover2, Tooltip/Tooltip2, and Dialogs. This is desirable for keyboard accessibility. Drawers still retain this behavior since they extend Overlay / OverlayableProps.

Reviewers should focus on:

Screenshot

2021-09-14 12 56 46

@blueprint-bot
Copy link

[core] feat(Overlay): add shouldReturnFocusOnClose prop, enable by default

Previews: documentation | landing | table

Copy link
Contributor

@styu styu left a comment

Choose a reason for hiding this comment

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

code looks + previews great! just a question on moving the prop

*
* @default true
*/
shouldReturnFocusOnClose?: boolean;
Copy link
Contributor

Choose a reason for hiding this comment

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

is this not a break?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's more like a QOL improvement for anyone that relies on keyboard navigation, since the focus would previously return to body. If you were manually setting focus in onClose, that still runs at the end of this change so your custom focus behavior will still work.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's not a break, DrawerProps extends from OverlayableProps!

Copy link
Contributor

Choose a reason for hiding this comment

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

ah, i missed that! 👍

@adidahiya adidahiya changed the title [core] feat(Overlay): add shouldReturnFocusOnClose prop, enable by default [core] feat(Overlay): add shouldReturnFocusOnClose prop Sep 14, 2021
@adidahiya adidahiya merged commit 2c71d6a into develop Sep 14, 2021
@adidahiya adidahiya deleted the ad/return-focus-to-popover-target branch September 14, 2021 22:00
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.

None yet

4 participants