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

experiment: native dialog component #15926

Merged
merged 8 commits into from
May 8, 2024

Conversation

lee-chase
Copy link
Member

@lee-chase lee-chase commented Mar 8, 2024

This adds an unstable_dialog component.

Created while investigating the possibility of creating a dialog component based on native dialog.

I included a side panel and tearsheet kind as part of the experiment.

Previously some questions have been raised here, perhaps it can be used to answer some of them.

#13807

Of note is that the modal form blocks the whole page. The modal dialogs can be nested.

Also quite a bit of the code added was to cope with the lack of support for
Unfortunately needed quite a bit of code to cope with the lack of support for some of the in/out styling

https://caniuse.com/mdn-css_at-rules_starting-style
https://caniuse.com/mdn-css_properties_display_is_transitionable

This much simpler code pen achieves a similar effect in Chrome and Edge
https://codepen.io/lee-chase/pen/QWPbRLp/9c8c3f23b42a395dc519839e12f5d647

NOTE: Use of :where([data-attri="xyz"]) done to see how it compares with BEM modifier overload. :where keeps the specificity low.

Copy link

netlify bot commented Mar 8, 2024

Deploy Preview for v11-carbon-react ready!

Name Link
🔨 Latest commit f66b9ee
🔍 Latest deploy log https://app.netlify.com/sites/v11-carbon-react/deploys/663bdb0a811de500083977be
😎 Deploy Preview https://deploy-preview-15926--v11-carbon-react.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@lee-chase
Copy link
Member Author

lee-chase commented Mar 11, 2024

The following branch contains nested dialogs. This appears to be problematic for various styling reasons:

  1. Dialog sizes are based on their containers (easiest to see in non-modal)
  2. Child dialogs sit obey normal stacking rules, so only appear over the top of content before them (see secondary button in image below).
  3. The child dialogs cause layout issues in their parents (plain dialog with 3 open see image).
image
  1. Several other styling issues not resolved try stacking tearsheet or side panel kinds. Although these are probably just a case of additional styling.
image image image

Modal versions work better.
image
image
image

https://github.com/lee-chase/carbon/tree/nativeDialogNest

Copy link
Member

@tay1orjones tay1orjones left a comment

Choose a reason for hiding this comment

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

Thanks so much for looking into this, I agree - it answers quite a few questions!

The tooltip on the close button appears on top of the modal as intended, so that's great news. Once we get the autoAlign stuff in I'll be eager to test how it works with this.

I have a few other thoughts below. I wouldn't be opposed to eventually merging this in, not exporting it for now, and hiding the stories from the storybook. I think it's valuable to keep this on deck and continue to experiment, and merging it in would avoid PRs sitting around getting stale.

packages/react/src/components/Dialog/index.js Outdated Show resolved Hide resolved
packages/react/src/components/Dialog/index.js Outdated Show resolved Hide resolved
packages/react/src/components/Dialog/index.js Outdated Show resolved Hide resolved
packages/react/src/components/Dialog/index.js Outdated Show resolved Hide resolved
packages/react/src/components/Dialog/index.js Outdated Show resolved Hide resolved
packages/react/src/components/FocusScope/index.js Outdated Show resolved Hide resolved
packages/react/src/components/Dialog/index.js Outdated Show resolved Hide resolved
@lee-chase lee-chase marked this pull request as ready for review April 16, 2024 10:38
@lee-chase lee-chase requested a review from a team as a code owner April 16, 2024 10:38
Copy link
Member

@tay1orjones tay1orjones left a comment

Choose a reason for hiding this comment

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

I think this puts the Dialog component on a much better track than it was previously! I still think we should keep it internal-only at this point and not export it or include it in storybook. If those changes are made I'd personally love to see this merged.

From here we can continue to iterate on this and explore the extra areas like building a new Modal implementation atop it, etc.

packages/styles/scss/components/_index.scss Outdated Show resolved Hide resolved
@lee-chase lee-chase requested a review from a team as a code owner May 8, 2024 08:58
@lee-chase lee-chase requested a review from tay1orjones May 8, 2024 08:58
@lee-chase
Copy link
Member Author

@tay1orjones accepted/made changes to remove from storybook.

Copy link
Member

@tay1orjones tay1orjones left a comment

Choose a reason for hiding this comment

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

I know it's silly but I'm genuinely excited about this one 😄
Native elements and standards feel so good.

Copy link
Member

@alisonjoseph alisonjoseph left a comment

Choose a reason for hiding this comment

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

Thanks for adding this 🥳

@alisonjoseph alisonjoseph added this pull request to the merge queue May 8, 2024
Merged via the queue into carbon-design-system:main with commit 00bbf9e May 8, 2024
20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants