-
Notifications
You must be signed in to change notification settings - Fork 646
fix(AnchedOverlay): hide overlay while positioning #1333
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/2mA2LQp2sWStLGvdG78yw1ko7y6w |
src/Overlay.tsx
Outdated
| } | ||
| } | ||
| visibility: ${props => props.visibility || 'visible'}; |
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.
Non-blocking: Would it be better to use a CSS variable for this (ref https://www.joshwcomeau.com/css/styled-components/#css-variables)?
I don’t feel strongly, since these styles already read props in other places (e.g. lines 60–62).
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.
Great catch! After a month with my head out of PRC, this didn't occur to me but is definitely a pattern I want to keep bringing in wherever possible. Fixed in 54d8783
smockle
left a comment
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.
Nice job tracking this down! Left a non-blocking comment above.
Non-blocking: Can we add a test for this? Either automatic, or a tall tale in Storybook for manual testing?
|
Added a test with the anchored overlay open, but I'm not sure I can recreate the scroll issues in JSDOM. Added a story as well for manual testing. |
090e669 to
88022ac
Compare
88022ac to
b66f1a4
Compare
b66f1a4 to
50b0d35
Compare
As noted in https://github.com/github/primer/issues/240, the updates from #1322 caused the
AnchoredOverlaycomponents to start positioning erratically. After further investigation, I found that they were actually being position at the bottom of the page (wherever thePortalis located) and rendered to determine their size before positioning. Because #1322 removed thevisibility: hiddenstyle, they were both visible and focusable before actually being positioned. The focus trap would activate, causing the page to scroll to the bottom, and then the position would be calculated from the anchor. This caused the menus in full page apps to be off significantly.Here I'm bringing back the
visibilityproperty onOverlayto allow for temporarily hidingOverlaysbefore positioning. I've tested this in the new Projects Beta and it's working as expected.I also found a small bug with
SelectPanels when they transitioned from aloading: truetoloading: falsestate. Because theFilteredActionListdoesn't render theListelement until loading is done, thelistContainerRefwas null when theuseFocusZonecall was made. We need to ensureuseFocusZonere-checks the ref whenloadingchanges, so I added it to thedependenciesparam.Closes https://github.com/github/primer/issues/240
Merge checklist