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): implement support for 3 button modals #7775

Merged
merged 11 commits into from
Feb 19, 2021

Conversation

emyarod
Copy link
Member

@emyarod emyarod commented Feb 10, 2021

(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
Copy link

netlify bot commented Feb 10, 2021

Deploy preview for carbon-elements ready!

Built with commit fcc0143

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

@netlify
Copy link

netlify bot commented Feb 10, 2021

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
Copy link
Member

aagonzales commented Feb 10, 2021

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
Copy link
Member Author

emyarod commented Feb 10, 2021

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
Copy link
Member Author

emyarod commented Feb 11, 2021

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

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.

Thanks for fixing the button text. Looks good!

@tw15egan
Copy link
Collaborator

tw15egan commented Feb 12, 2021

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
Copy link
Collaborator

@tw15egan tw15egan left a comment

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