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

fix(Modal): content spacing styles #8495

Closed
wants to merge 6 commits into from

Conversation

jnm2377
Copy link
Contributor

@jnm2377 jnm2377 commented Apr 23, 2021

Closes #8373

  • Adds padding only to paragraph/text within modal. All other elements span full width.
  • xs modal content always spans full width, even paragraphs/text.

@jnm2377 jnm2377 requested a review from aagonzales April 23, 2021 13:52
@jnm2377 jnm2377 requested a review from a team as a code owner April 23, 2021 13:52
@netlify
Copy link

netlify bot commented Apr 23, 2021

Deploy preview for carbon-elements ready!

Built with commit d8cdf61

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

@netlify
Copy link

netlify bot commented Apr 23, 2021

Deploy preview for carbon-components-react ready!

Built with commit d8cdf61

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

@netlify
Copy link

netlify bot commented Apr 23, 2021

Deploy preview for carbon-elements ready!

Built with commit ae95e90

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

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 good! 👍

@netlify
Copy link

netlify bot commented Apr 23, 2021

Deploy preview for carbon-components-react ready!

Built with commit ae95e90

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

@aagonzales
Copy link
Member

Not sure if this is related to this PR or not but the fade at the bottom of the content isn't showing up when there is scrolling content. I'm not seeing it in regular story book so its probably not related, just wanted to check.

@tw15egan
Copy link
Member

tw15egan commented Apr 23, 2021

@jnm2377 it seems like this update will make the hasForm prop redundant, since all non-p elements will now span full width.

Should we add a deprecate notice to the prop, and leave a coment to remove the .#{$prefix}--modal-content--with-form in V11?

Other changes look great!

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.

LGTM!

Copy link
Member

@emyarod emyarod left a comment

Choose a reason for hiding this comment

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

should the xs modal paragraphs have no additional padding or am I misinterpreting this note? #8373 (comment)

@jnm2377
Copy link
Contributor Author

jnm2377 commented Apr 29, 2021

@tw15egan good call, updated with deprecation warning 👍🏽

@jnm2377
Copy link
Contributor Author

jnm2377 commented Apr 29, 2021

should the xs modal paragraphs have no additional padding or am I misinterpreting this note? #8373 (comment)

@emyarod I'll defer to @aagonzales on this. I left the xs padding as is.

@aagonzales
Copy link
Member

Oh good catch. The xs small one if good but the sm needs a breakpoint change at 1056 to also have the paragraph content span 100%

@jnm2377 jnm2377 requested a review from a team as a code owner April 29, 2021 21:18
@tw15egan
Copy link
Member

tw15egan commented Apr 29, 2021

It looks like XS still has some padding when the modal is displayed on smaller screens. It looks like the rest of the size variants don't have this padding.

XS
Screen Shot 2021-04-29 at 3 49 29 PM

All other variants
Screen Shot 2021-04-29 at 3 49 46 PM

@jnm2377 jnm2377 closed this Sep 20, 2021
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.

modal bug: inputs should span the full width
5 participants