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

feat(Modal): add isFullWidth prop #12462

Merged
merged 2 commits into from
Nov 3, 2022

Conversation

tw15egan
Copy link
Collaborator

@tw15egan tw15egan commented Nov 1, 2022

Closes #11933

Adds a new isFullWIdth prop that removes any margin and padding placed on the modal-content class.

Changelog

New

  • isFullWidth prop and associated styles for Modal and ComposedModal that removes margin and padding.
  • e2e tests added to catch regressions
  • updated snapshots

Testing / Reviewing

Was unsure if we also wanted to remove the padding/margin from the headers; for now they are still there because they match the spec images

  • Go to ComposedModal and Modal and ensure the Full Width example matches the spec images.

@netlify
Copy link

netlify bot commented Nov 1, 2022

Deploy Preview for carbon-components-react ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit fc9a2dd
🔍 Latest deploy log https://app.netlify.com/sites/carbon-components-react/deploys/6363b008df12150009ba8c09
😎 Deploy Preview https://deploy-preview-12462--carbon-components-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 settings.

@netlify
Copy link

netlify bot commented Nov 1, 2022

Deploy Preview for carbon-elements ready!

Name Link
🔨 Latest commit fc9a2dd
🔍 Latest deploy log https://app.netlify.com/sites/carbon-elements/deploys/6363b00851fc4d0008cefc00
😎 Deploy Preview https://deploy-preview-12462--carbon-elements.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 settings.

@aagonzales
Copy link
Member

The full-width stories look good, but I had a follow-up question after seeing the Playground example. Would a user be able to mix full width and the indented style? Or is it an either-or situation? Like below

image

@tw15egan
Copy link
Collaborator Author

tw15egan commented Nov 1, 2022

I think they would still be able to do that; they would just have to add padding independently. This just removes any padding we ship to prevent our styles from blocking any designs since we only place padding on the modal body container.

Copy link
Contributor

@shixiedesign shixiedesign left a comment

Choose a reason for hiding this comment

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

Thank you! Looks great and thanks for the ⚡ speed. I'm wondering if this creates additional work for @carbon-design-system/carbon-platform-designers to add guidance on when to use the isFullwidth prop...? This PR looks good to me though! Thank you!

@aagonzales
Copy link
Member

You bring up a good point @shixiedesign we need to add guidance on the website now. It would actually be the system squad who adds it (platform is pulling markdown from the original site atm). I've made an issue so we don't forget.

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.

Looks great!

@kodiakhq kodiakhq bot merged commit 3e192c0 into carbon-design-system:main Nov 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

[Feature Request]: Modal to allow full width & full height content
5 participants