-
Notifications
You must be signed in to change notification settings - Fork 638
Refactor Dialog component #955
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
Conversation
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/primer/primer-components/ka7rqpq3w |
Loving this new direction! ❤️ I haven't finished reviewing all the code yet but looks like focus trapping isn't quite working. Clicking inside the dialog then tabbing leads to this: Kapture.2021-01-05.at.14.27.06.mp4 |
@colebemis nice catch, I added a negative tab index to the modal container and that seems to have fixed it 🙌 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚀
src/Button/ButtonClose.js
Outdated
cursor: pointer; | ||
|
||
&:focus { | ||
box-shadow: 0 0 0 3px rgba(139, 148, 158, 0.3); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just curious, where'd these focus styles come from?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm good question - I copied over the focus box shadow used in the close button in toasts, but those are on a dark background in light mode, which is why its grey. I would probably be better to used the primary box-shadow-focus here instead: https://github.com/primer/css/blob/master/src/support/variables/misc.scss#L23
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The official Primer .button-close
doesn't include a focus outline, but it probably should, so I'm going to use the default shadow here
This reverts commit 092655e.
This PR refactors the Dialog component
@reach/dialog
dependency which doesn't currently support React 17 and was bloating our bundle size.narrow
andwide
for easy setting of default widthsinitialFocusRef
to allow users to focus an item other than the close button when the Dialog is openedreturnFocusRef
to return focus to the proper element when Dialog is closedReduces our bundle size by about 19% (23% gzipped)! 🎉 🎉 🎉
Technically there are no breaking changes in this PR, but we are adding a new
returnFocusRef
that is used to ensure that focus is restored to the proper place when the Dialog is closed. This wasn't working before, so upgrading PRC to a release with these changes won't break anything, but going forward we'd like users to include areturnFocusRef
. This prop won't be required (to accommodate applications that want to manage focus when the Dialog closes themselves) but highly recommended if you aren't managing focus yourself.Closes #930
Merge checklist
index.d.ts
) if necessaryTake a look at the What we look for in reviews section of the contributing guidelines for more information on how we review PRs.