Skip to content

feat(Modal): implement support for 3 button modals#7775

Merged
kodiakhq[bot] merged 11 commits into
carbon-design-system:masterfrom
emyarod:4607-3-button-modal
Feb 19, 2021
Merged

feat(Modal): implement support for 3 button modals#7775
kodiakhq[bot] merged 11 commits into
carbon-design-system:masterfrom
emyarod:4607-3-button-modal

Conversation

@emyarod

@emyarod emyarod commented Feb 10, 2021

Copy link
Copy Markdown
Member

(tests will be updated when we have confirmed if the API changes are good to go)

--

Related #4607

This PR adds support for 3 button modals according to the latest modal spec. Since our existing Modal API expects only 1 primary and 1 secondary button, a secondaryButtons prop is introduced which can be used in place of the secondaryButtonText and onSecondarySubmit props. The secondaryButtons prop expects an array of up to 2 objects with the following shape:

{
  buttonText: PropTypes.string,
  onClick: PropTypes.func,
}

if an onClick handler is not supplied to the secondary buttons in a ComposedModal it will fall back to the internal handleRequestClose method by default

Changelog

New

  • secondaryButtons prop
  • 3 button modal styles (not including linear directional modals)

Testing / Reviewing

Confirm the 3 button modal styles appear correct and there are no visual regressions with existing supported modal styles

@netlify

netlify Bot commented Feb 10, 2021

Copy link
Copy Markdown

Deploy preview for carbon-elements ready!

Built with commit fcc0143

https://deploy-preview-7775--carbon-elements.netlify.app

@netlify

netlify Bot commented Feb 10, 2021

Copy link
Copy Markdown

Deploy preview for carbon-components-react ready!

Built without sensitive environment variables with commit fcc0143

https://deploy-preview-7775--carbon-components-react.netlify.app

@aagonzales

aagonzales commented Feb 10, 2021

Copy link
Copy Markdown
Contributor

The primary button is reflowing weird. It should keep a consistent top margin. Is this just a button bug unrealted to this PR?
image

@emyarod emyarod requested a review from a team as a code owner February 10, 2021 20:49
@emyarod

emyarod commented Feb 10, 2021

Copy link
Copy Markdown
Member Author

that's just directly related to the width change, since instead of 50% width they're now 25% width. if you increase the size of your viewport it would be what you'd normally expect I think

image

@emyarod emyarod force-pushed the 4607-3-button-modal branch from 819b9ca to 1bb7ece Compare February 11, 2021 19:44
@emyarod

emyarod commented Feb 11, 2021

Copy link
Copy Markdown
Member Author

modified button text vertical alignment for 3 button modals only after discussion with @aagonzales

@aagonzales aagonzales left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for fixing the button text. Looks good!

Comment thread packages/react/src/components/Modal/Modal-story.js
@tw15egan

tw15egan commented Feb 12, 2021

Copy link
Copy Markdown
Contributor

Looks really great! Do you think we can add a quick blurb to the .mdx files to showcase how to use the 3 button variant of Modal and ComposedModal? The doc table doesn't seem to showcase the expected config objects / props

Screen Shot 2021-02-12 at 12 39 51 PM

@emyarod emyarod force-pushed the 4607-3-button-modal branch from 1bb7ece to 3dfdc1d Compare February 12, 2021 20:45

@tw15egan tw15egan left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM 👍 ✅ 🎉

@emyarod emyarod force-pushed the 4607-3-button-modal branch from cb9a3d0 to 58713ff Compare February 16, 2021 16:19
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.

4 participants