feat(Overlay): slide away from anchor based on position #1322

dgreif · 2021-06-23T23:09:59Z

The main goal here is to prevent scrollbars from showing briefly when an AnchoredOverlay is opened. This can happen if the Overlay portion ends up slightly outside the currently visible area upon creation, or because of the slide animation. There are two main changes to correct this behavior

Pass anchorSide from useAnchoredPosition all the way into Overlay. With this information, we can infer which direction the Overlay should "slide" when it is opened. We always want to to start slightly overlapping the anchor and then move into position. Previous behavior had it always sliding "up", which is only desired if the Overlay is positioned above its anchor. If not passed an anchorSide, there will now be no slide animation.
Remove the visibility setting from Overlay. This was a bandaid to handle scenarios where the Overlay contents would briefly flash before moving into the correct position. While setting visibility removed the main visible flash, it did not prevent scrollbars from flashing if the unpositioned content happened to overflow the current frame. The more correct solution here is to replace a few useEffect with useLayoutEffect, which allows AnchoredOverlay to create the Overlay, calculate position, and re-position the Overlay before the browser paints.

Closes #1231

Screenshots

Side	Animation
Bottom
Right
Top
Left
No Side Provided

Merge checklist

changeset-bot · 2021-06-23T23:10:01Z

⚠️ No Changeset found

Latest commit: 39b1d63

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

vercel · 2021-06-23T23:10:01Z

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/primer/primer-components/6tF9Rmx2gZnDZ9VzRhUvqK6EtVa7
✅ Preview: https://primer-components-git-overlay-transition-from-anchor-primer.vercel.app

smockle · 2021-06-25T03:39:24Z

From @dgreif in #1322 (comment):

The main goal here is to prevent scrollbars from showing briefly when an AnchoredOverlay is opened.

To clarify: This PR prevents the page from flashing scrollbars (as demonstrated in #1231). The scrollbar flash added via #1276 to SelectPanel is still desired and has been retained.

src/AnchoredOverlay/AnchoredOverlay.tsx

src/Overlay.tsx

- https://dequeuniversity.com/rules/axe/4.1/aria-dialog-name - https://dequeuniversity.com/rules/axe/4.2/presentation-role-conflict - https://www.w3.org/TR/wai-aria-practices-1.1/examples/menu-button/menu-button-links.html First, 'Overlay's aren’t 'listbox'es, because (when used in 'DropdownMenu' or 'ActionMenu') they contain 'menuitem's, 'menuitemradio's, or 'menuitemcheckbox'es. Second, 'Overlay's aren’t 'dialog's, because (as demonstrated in the WAI ARIA practices page linked above), 'menu's need not be contained in a 'dialog', and also (as noted in the 'aria-dialog-name' link above), 'dialog's must have an 'aria-label', 'aria-labelledby', or 'title', but neither 'DropdownMenu' nor 'ActionMenu' have any kind of header element that could be used for this. Third, if 'Overlay's are 'none', they can’t be focusable (as noted in the 'presentation-role-conflict' link above), but one of our hooks (maybe 'FocusTrap', maybe 'FocusZone') was setting 'tabIndex' to '0' (in the test component), because it did not contain a focusable child. This PR adds a focusable button child so the 'none' 'Overlay' container won’t receive 'tabIndex' '0'.

* feat(Overlay): slide away from anchor based on position * fix: handle position changes when re-opening AnchoredOverlay * refactor: use js animation for slide to fix safari * fix: Tests were failing with Axe violations - https://dequeuniversity.com/rules/axe/4.1/aria-dialog-name - https://dequeuniversity.com/rules/axe/4.2/presentation-role-conflict - https://www.w3.org/TR/wai-aria-practices-1.1/examples/menu-button/menu-button-links.html First, 'Overlay's aren’t 'listbox'es, because (when used in 'DropdownMenu' or 'ActionMenu') they contain 'menuitem's, 'menuitemradio's, or 'menuitemcheckbox'es. Second, 'Overlay's aren’t 'dialog's, because (as demonstrated in the WAI ARIA practices page linked above), 'menu's need not be contained in a 'dialog', and also (as noted in the 'aria-dialog-name' link above), 'dialog's must have an 'aria-label', 'aria-labelledby', or 'title', but neither 'DropdownMenu' nor 'ActionMenu' have any kind of header element that could be used for this. Third, if 'Overlay's are 'none', they can’t be focusable (as noted in the 'presentation-role-conflict' link above), but one of our hooks (maybe 'FocusTrap', maybe 'FocusZone') was setting 'tabIndex' to '0' (in the test component), because it did not contain a focusable child. This PR adds a focusable button child so the 'none' 'Overlay' container won’t receive 'tabIndex' '0'. * fix: Resolve lint errors Co-authored-by: Clay Miller <[email protected]>

* add utility props to Box * update box docs * export box props * update snapshots * Create green-worms-nail.md * AvatarStack story in storybook * Update .changeset/green-worms-nail.md Co-authored-by: Cole Bemis <[email protected]> * Update docs/content/Box.md Co-authored-by: Cole Bemis <[email protected]> * Remove duplicate border system prop definitions * Remove duplicate grid system props definitions * Update FlexProps definition * Remove duplicate position system prop definitions * Update Box documentation * Update BoxProps * Update Box docs * Update Box props * fix: Type 'DropdownButton' as 'button' (#1318) * fix: Type 'DropdownButton' as 'button' * chore: Update snapshots * chore: Set test directory via config rather than flag (#1319) * feat(useFocusZone): update active-descendant on mousemove (#1308) * fix: Split `<Item>` labels (#1320) * fix: Separate 'Item' content into 'label' and 'description' * fix: Only add 'aria-describedby' when 'description' exists * fix: Memoize 'id' so 'Item's and labels match * fix: Don’t rely on 'id' which is possibly not globally-unique * fix: Restore semi-full-width 'Item' dividers, without giving up the semantic nesting. By “semantic nesting”, I mean that the 'Item' label and description are now siblings, which is preferable to the previous implementation, where the description node was a child of the label node. As a general principle, we should align DOM hierarchies with information hierarchies. An analogy: If I were using a bulleted list to describe a dog, I would not indent its breed as a second-level bullet beneath the bullet for its name, because a dog’s breed is not dependent/derived data from its name. Similarly, description is not dependent/derived from label, and so should not be nested in DOM. * fix: Reduce styled-components class permutations. https://www.joshwcomeau.com/css/styled-components/ * feat(Overlay): slide away from anchor based on position (#1322) * feat(Overlay): slide away from anchor based on position * fix: handle position changes when re-opening AnchoredOverlay * refactor: use js animation for slide to fix safari * fix: Tests were failing with Axe violations - https://dequeuniversity.com/rules/axe/4.1/aria-dialog-name - https://dequeuniversity.com/rules/axe/4.2/presentation-role-conflict - https://www.w3.org/TR/wai-aria-practices-1.1/examples/menu-button/menu-button-links.html First, 'Overlay's aren’t 'listbox'es, because (when used in 'DropdownMenu' or 'ActionMenu') they contain 'menuitem's, 'menuitemradio's, or 'menuitemcheckbox'es. Second, 'Overlay's aren’t 'dialog's, because (as demonstrated in the WAI ARIA practices page linked above), 'menu's need not be contained in a 'dialog', and also (as noted in the 'aria-dialog-name' link above), 'dialog's must have an 'aria-label', 'aria-labelledby', or 'title', but neither 'DropdownMenu' nor 'ActionMenu' have any kind of header element that could be used for this. Third, if 'Overlay's are 'none', they can’t be focusable (as noted in the 'presentation-role-conflict' link above), but one of our hooks (maybe 'FocusTrap', maybe 'FocusZone') was setting 'tabIndex' to '0' (in the test component), because it did not contain a focusable child. This PR adds a focusable button child so the 'none' 'Overlay' container won’t receive 'tabIndex' '0'. * fix: Resolve lint errors Co-authored-by: Clay Miller <[email protected]> * Generate prop documentation (#1323) * Add new filesystem source * Add component metadata type * Create Props component * Update props table * Handle empty and error states * Add required label * Update required prop styles * Clean up code comments * Remove filesystem plugin * Remove extra markdown file * Add component comment Co-authored-by: Clay Miller <[email protected]> * Improve treeshaking by setting package.json sideEffects (#1332) * fix: mark sideEffects free * fix: update sideEffects delcaration in package.json to improve treeshaking * fix: update sideEffects delcaration in package.json to improve treeshaking * fix: BaseStyles doesnt use sideeffects * chore: add changeset * Update Box documentation * Update BoxProps * Update Box docs * Update Box props * Remove AvatarStack story * Update .changeset/green-worms-nail.md Co-authored-by: Cole Bemis <[email protected]> Co-authored-by: Clay Miller <[email protected]> Co-authored-by: Dusty Greif <[email protected]> Co-authored-by: Matthew Costabile <[email protected]>

dgreif force-pushed the overlay-transition-from-anchor branch from 210b535 to 8374dd5 Compare June 24, 2021 22:26

vercel bot temporarily deployed to Preview June 24, 2021 22:26 Inactive

dgreif marked this pull request as ready for review June 24, 2021 22:30

dgreif requested a review from smockle June 24, 2021 22:31

smockle reviewed Jun 25, 2021

View reviewed changes

src/AnchoredOverlay/AnchoredOverlay.tsx Show resolved Hide resolved

smockle reviewed Jun 25, 2021

View reviewed changes

src/Overlay.tsx Outdated Show resolved Hide resolved

smockle reviewed Jun 25, 2021

View reviewed changes

src/Overlay.tsx Outdated Show resolved Hide resolved

smockle reviewed Jun 25, 2021

View reviewed changes

src/Overlay.tsx Outdated Show resolved Hide resolved

vercel bot temporarily deployed to Preview June 25, 2021 18:57 Inactive

dgreif added 3 commits June 25, 2021 16:18

feat(Overlay): slide away from anchor based on position

7e2feaa

fix: handle position changes when re-opening AnchoredOverlay

5835f86

refactor: use js animation for slide to fix safari

db69e07

dgreif force-pushed the overlay-transition-from-anchor branch from 2b7953c to db69e07 Compare June 25, 2021 23:19

vercel bot temporarily deployed to Preview June 25, 2021 23:19 Inactive

vercel bot temporarily deployed to Preview June 29, 2021 16:23 Inactive

fix: Resolve lint errors

39b1d63

vercel bot temporarily deployed to Preview June 29, 2021 19:31 Inactive

smockle approved these changes Jun 29, 2021

View reviewed changes

smockle merged commit 12e9bf2 into main Jun 29, 2021

smockle deleted the overlay-transition-from-anchor branch June 29, 2021 21:10

dgreif mentioned this pull request Jul 20, 2021

fix(AnchedOverlay): hide overlay while positioning #1333

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

feat(Overlay): slide away from anchor based on position #1322

feat(Overlay): slide away from anchor based on position #1322

Uh oh!

dgreif commented Jun 23, 2021

Uh oh!

changeset-bot bot commented Jun 23, 2021 •

edited

Loading

Uh oh!

vercel bot commented Jun 23, 2021 •

edited

Loading

Uh oh!

smockle commented Jun 25, 2021 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

feat(Overlay): slide away from anchor based on position #1322

feat(Overlay): slide away from anchor based on position #1322

Uh oh!

Conversation

dgreif commented Jun 23, 2021

Screenshots

Merge checklist

Uh oh!

changeset-bot bot commented Jun 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

⚠️ No Changeset found

Uh oh!

vercel bot commented Jun 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

smockle commented Jun 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

changeset-bot bot commented Jun 23, 2021 •

edited

Loading

vercel bot commented Jun 23, 2021 •

edited

Loading

smockle commented Jun 25, 2021 •

edited

Loading