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.

Sign up for GitHub

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

Jump to bottom

DialogPrimitive - Fix issue with box-sizing inheritance #2442

Merged
merged 3 commits into from
Sep 23, 2024
Merged

DialogPrimitive - Fix issue with box-sizing inheritance #2442

merged 3 commits into from
Sep 23, 2024

Conversation

didoo
Copy link
Contributor

@didoo didoo commented Sep 23, 2024 •
edited
Loading

📌 Summary

In #2211 I (specifically @didoo) have introduced a change to the Modal/Flyout components, to use a newly created DialogPrimitive that could be used as standalone component/primitive as well as sub-component

Because of this, I decided to reset the default styles applied to the "root" element of the DialogPrimitive, which is a <dialog> HTML element, using a all: unset CSS rule, and then apply the desired styles at Modal/Flyout level (while sharing the common styles in the DialogPrimitive CSS styles, declaring them just after the all: unset reset:

:where(.hds-dialog-primitive__wrapper) {
  // -----------
  all: unset; // removes default dialog & inherited styles. Must be the first style to be applied
  // -----------
  display: flex;
  flex-direction: column;
  width: 100%;
  height: 100%;
  margin: 0;
  padding: 0;
  background: var(--token-color-surface-primary);
}

The change was described as "non-breaking" in the original PR.

Thanks to @aklkv raising the issue in a Slack thread, we've come to realize that this actually introduced a regression in the UI of the Modal in some of our components: depending on where the CSS reset for the box-sizing property was declared in the order of imports, some of the UI elements assumed a box-sizing: content-box property (thanks to the *, *::before, *::after { box-sizing: inherit; } rule) and with in some cases the padding applied to them was added to the height, causing them to blow up (see button in the screenshot below):

image

We missed this detail, both in implementation as well as in review, because the HDS showcase the reset is declared after the import of the HDS components styles, so the box-sizing applied to <dialog> is inherited from the <html> element, which is set to border-box in the reset:

@import "@hashicorp/design-system-components";
@import "@hashicorp/design-system-power-select-overrides";

// global declarations

@import "./tokens";
@import "./layout";
@import "./typography";
@import "./globals";  // ← the box-sizing CSS reset is here

while in Cloud UI is declared before the import of the CSS

@use "resets";  // ← the box-sizing CSS reset is here
@use "lists";

// Components and helpers provided by the design system
// Notice: the full path is declared in the `hcp/ember-cli-build.js` file, under `sassOptions.includePaths`
@use "helpers/typography.css";
@use "@hashicorp/design-system-components.css";

🛠️ Detailed description

In this PR I have:

  • removed all: unset declaration from hds-dialog-primitive__wrapper and replaced with explicit overrides
  • updated version-history and tags in website documentation

Notice: there was a small visual regression in Percy but only in a mobile viewport and only for a use case, and was likely a fluke.

Preview: https://hds-showcase-git-3869-fix-dialog-primitive-box-sizing-hashicorp.vercel.app/components/modal

🔗 External links

Jira ticket: https://hashicorp.atlassian.net/browse/HDS-3869

👀 Component checklist

💬 Please consider using conventional comments when reviewing this PR.

@vercel Vercel
Copy link

vercel bot commented Sep 23, 2024 •
edited
Loading

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
hds-showcase ✅ Ready (Inspect) Visit Preview Sep 23, 2024 11:24am
hds-website ✅ Ready (Inspect) Visit Preview Sep 23, 2024 11:24am

@didoo didoo force-pushed the 3869-fix-dialog-primitive-box-sizing branch from 591db56 to fc893b8 Compare September 23, 2024 11:10
@hashibot-hds hashibot-hds added the docs-website Content updates to the documentation website label Sep 23, 2024
@didoo didoo requested review from alex-ju, KristinLBradley and a team September 23, 2024 13:51
shleewhite
shleewhite approved these changes Sep 23, 2024
View reviewed changes
packages/components/src/styles/components/dialog-primitive.scss
all: unset; // removes default dialog & inherited styles. Must be the first style to be applied
// -----------
// override default `<dialog>` styles
position: initial;
Copy link
Contributor

Choose a reason for hiding this comment

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

I like that we're being explicit about what we are overriding from <dialog>, makes it easier to understand!

@didoo didoo merged commit 577ff39 into main Sep 23, 2024
14 checks passed
@didoo didoo deleted the 3869-fix-dialog-primitive-box-sizing branch September 23, 2024 17:19
@hashibot-hds hashibot-hds mentioned this pull request Sep 23, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@KristinLBradley KristinLBradley KristinLBradley approved these changes

@shleewhite shleewhite shleewhite approved these changes

@alex-ju alex-ju Awaiting requested review from alex-ju

Assignees
No one assigned
Labels
docs-website Content updates to the documentation website packages/components
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@didoo @shleewhite @KristinLBradley @hashibot-hds