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

feat(Modal): add slug to Modal, ComposedModal #15350

Merged
merged 7 commits into from
Dec 13, 2023
Merged

feat(Modal): add slug to Modal, ComposedModal #15350

merged 7 commits into from
Dec 13, 2023

Conversation

tw15egan
Copy link
Collaborator

@tw15egan tw15egan commented Dec 7, 2023

Closes #15192

Adds in the slug prop to Modal and ComposedModal.

Changelog

New

  • slug prop added to Modal and ComposedModal
  • slugOverlay tokens added to provide a lighter overlay in light themes
  • Modal and ComposedModal added to the Examples folder inside Slug in a storybook. One shows with full-width button (gradient should start at 64px), and the other has 3 buttons (gradient should start at 0px)

Changed

  • Added an offset to the ai-gradient mixin so that the gradient can start at a different value than 0 when buttons are full width inside the modal
  • Overlay changed to use new tokens
  • Box shadow added to both Modal variants when slug is provided
  • Fixed an issue that was causing the slug callout text not to use the custom story styles

Removed

  • overflow: hidden on .cds--modal-container was causing the slug to be cut off; Contents inside still seem to scroll properly

Questions

  • This is currently using the ai-gradient, but should it be using the callout-gradient that is used in Tile and the Slug callout?
  • I pulled the drop-shadow values directly from Figma and mapped them to box-shadow, are these correct?

Testing / Reviewing

Go to unstable__slug --> Examples --> Modal as well as ComposedModal and ensure everything renders as expected

@tw15egan tw15egan requested review from a team as code owners December 7, 2023 19:59
@netlify Netlify
Copy link

netlify bot commented Dec 7, 2023

✅ Deploy Preview for v11-carbon-react ready!

Name Link
🔨 Latest commit fe62de0
🔍 Latest deploy log https://app.netlify.com/sites/v11-carbon-react/deploys/65722430b9e52c0007d02553
😎 Deploy Preview https://deploy-preview-15350--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.

@netlify Netlify
Copy link

netlify bot commented Dec 7, 2023 •
edited
Loading

✅ Deploy Preview for v11-carbon-react ready!

Name Link
🔨 Latest commit b27a526
🔍 Latest deploy log https://app.netlify.com/sites/v11-carbon-react/deploys/657362a01cefee00083e40b3
😎 Deploy Preview https://deploy-preview-15350--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.

@aagonzales
Copy link
Member

@tw15egan

Answers

  1. Use the callout-gradient instead of the ai-gradient in modal
  2. Whoops, this is my bad for not catching this when Rich was showing me this but we actually went back to a more traditional modal style with this AI work (instead of this original shadow work). So no shadow at all and then just the normal overlay. And then remove any transparency on the background layers so it comes out looking like this:
image

I can get you a better spec tomorrow if you need it.

aagonzales
aagonzales reviewed Dec 8, 2023
View reviewed changes
Copy link
Member

@aagonzales aagonzales left a comment

Choose a reason for hiding this comment

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

Yeah that's looking correct except there is a visual difference between the composed and default modal. I think the default modal still has transparency on it somewhere which is why its a little darker.

image

@tw15egan
Copy link
Collaborator Author

tw15egan commented Dec 8, 2023

@aagonzales fixed 👍

aagonzales
aagonzales approved these changes Dec 8, 2023
View reviewed changes
Copy link
Member

@aagonzales aagonzales left a comment

Choose a reason for hiding this comment

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

Ignoring the problems in the gray 10 theme that will hopefully be resolved with v2 visuals, this is good to go.

tay1orjones
tay1orjones approved these changes Dec 13, 2023
View reviewed changes
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.

LGTM 👍

andreancardona
andreancardona approved these changes Dec 13, 2023
View reviewed changes
Copy link
Contributor

@andreancardona andreancardona left a comment

Choose a reason for hiding this comment

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

just tested, looks good!

@github-actions github-actions bot added this pull request to the merge queue Dec 13, 2023
@tay1orjones tay1orjones removed this pull request from the merge queue due to a manual request Dec 13, 2023
@tay1orjones tay1orjones merged commit 3748b40 into carbon-design-system:main Dec 13, 2023
22 checks passed
danoro96 pushed a commit to danoro96/carbon that referenced this pull request Jan 18, 2024
@tw15egan @danoro96
feat(Modal): add slug to Modal, ComposedModal (carbon-design-system#1…
e84c82d 
…5350)

* feat(Modal): add slug to Modal, ComposedModal

* test(snapshot): update snapshots

* docs(Slug): add slug to controls table

* fix(Modal): remove shadows, use callout gradient

* test(snapshot): update snapshots

* fix(Modal): adjust background, fix conditional class
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andreancardona andreancardona andreancardona approved these changes

@aagonzales aagonzales aagonzales approved these changes

@tay1orjones tay1orjones tay1orjones approved these changes

@guidari guidari Awaiting requested review from guidari guidari was automatically assigned from carbon-design-system/developers-system

@RichKummer RichKummer Awaiting requested review from RichKummer

Assignees
No one assigned
Labels
status: ready to merge 🎉
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add AI enhancements to React Modal
4 participants
@tw15egan @aagonzales @tay1orjones @andreancardona