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

Hide overflow from Overlay #1248

Merged
merged 2 commits into from
May 25, 2021
Merged

Hide overflow from Overlay #1248

merged 2 commits into from
May 25, 2021

Conversation

dgreif
Copy link
Member

@dgreif dgreif commented May 24, 2021

This partially reverts commit 681bbe6. I talked through these changes at length with @smockle, but wanted to document some of the main points here.

The new Dialog component is intentionally allowing overflow (no overflow: hidden) because the main use case, ConfirmationDialog, has buttons that go right to the edge of the Dialog. When those buttons are focused, the default browser outline falls outside the Dialog. If overflow: hidden is set, the outlines get chopped off. The only way to fix this specific issue is by allowing overflow. See #1203 for more details.

Overflow Hidden Overflow Allowed
before after

In 681bbe6, we decided to make a similar adjustment to the Overlay component. The driving factor at the time was outline clipping, similar to what we saw in Dialog, but specifically when ActionList.Items were focused within an Overlay. Additionally, we wanted consistency between Dialog and Overlay.

After some new use cases have come up, such as scroll bars in SelectPanel, it's becoming clear that the overflow content will often cause the bottom corners to lose the correct border-radius, like this:

image

Rather than expecting Overlay consumers to round their corners to perfectly match the Overlay border radius, @smockle and I concluded it would be best to bring back overflow: hidden in the Overlay. This gives us accurate border-radius, and ensures that Overlay contents aren't otherwise spilling out. We discussed some upcoming use cases, such as Tooltips in Overlays, or nested ActionMenus as counter scenarios where overflow might be desirable. In all the cases we could think of, the real solution is to have those new overlaying elements be created in a Portal, as the Overlay is now. If portaled, those child elements will actually be outside the Overlay in the DOM, and won't be restricted in any way by the overflow: hidden.

This partially reverts commit 681bbe6
@dgreif dgreif requested review from smockle and colebemis May 24, 2021 22:36
@changeset-bot
Copy link

changeset-bot bot commented May 24, 2021

🦋 Changeset detected

Latest commit: a36bf29

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@primer/components Patch

Not sure what this means? Click here to learn what changesets are.

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

@vercel
Copy link

vercel bot commented May 24, 2021

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/4aEdV6y8SXE4ZJk1vYDmWwp37yYW
✅ Preview: https://primer-components-git-overlay-overflow-hidden-primer.vercel.app

@vercel vercel bot temporarily deployed to Preview May 25, 2021 03:16 Inactive
@dgreif dgreif enabled auto-merge (squash) May 25, 2021 03:16
@dgreif dgreif merged commit a7fe32c into main May 25, 2021
@dgreif dgreif deleted the overlay-overflow-hidden branch May 25, 2021 03:20
@github-actions github-actions bot mentioned this pull request May 25, 2021
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

2 participants