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): updates content spacing #9700

Merged
merged 4 commits into from
Oct 4, 2021

Conversation

jnm2377
Copy link
Contributor

@jnm2377 jnm2377 commented Sep 20, 2021

Closes #8373

This PR essentially updates the spacing within modals so that any content that is not text spans full width (with 16px padding), and text gets 20% right padding.

(FYI: I previously had a different PR up for this, which I closed).

SPECS:

xs

  • everything spans full width, at all breakpoints

sm

  • text gets 20% padding above 1056 (lg) breakpoint
  • everything spans full width below 1056 breakpoint

md, lg, xl

  • text gets 20% right padding
  • everything else spans full width

Changelog

  • consolidates a lot of the styles bc they were very repetitive and confusing
  • deprecate hasForm
  • add 20% right padding to p within modal content

Testing / Reviewing

  • checkout Modal story and review padding for all the different sized modals using the specs above

@jnm2377 jnm2377 requested a review from aagonzales September 20, 2021 16:37
@jnm2377 jnm2377 requested review from a team as code owners September 20, 2021 16:37
@netlify
Copy link

netlify bot commented Sep 20, 2021

✔️ Deploy Preview for carbon-react-next ready!

🔨 Explore the source changes: 91708ff

🔍 Inspect the deploy log: https://app.netlify.com/sites/carbon-react-next/deploys/615b2a4dd94fea00075dbcae

😎 Browse the preview: https://deploy-preview-9700--carbon-react-next.netlify.app

@netlify
Copy link

netlify bot commented Sep 20, 2021

✔️ Deploy Preview for carbon-components-react ready!
Built without sensitive environment variables

🔨 Explore the source changes: 91708ff

🔍 Inspect the deploy log: https://app.netlify.com/sites/carbon-components-react/deploys/615b2a4d57cdb400088df94c

😎 Browse the preview: https://deploy-preview-9700--carbon-components-react.netlify.app

@netlify
Copy link

netlify bot commented Sep 20, 2021

✔️ Deploy Preview for carbon-elements ready!

🔨 Explore the source changes: 91708ff

🔍 Inspect the deploy log: https://app.netlify.com/sites/carbon-elements/deploys/615b2a4e70623b000879e74d

😎 Browse the preview: https://deploy-preview-9700--carbon-elements.netlify.app

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

@joshblack
Copy link
Contributor

@jnm2377 how does this work for different types of content in the modal? If I remember right, @tw15egan brought this up over in: #8373 (comment) and I was curious what your take on that was

@jnm2377
Copy link
Contributor Author

jnm2377 commented Sep 22, 2021

@joshblack I added padding that was specific to p elements within modal body, and everything else in the body spans full width. It seems like design is ok with other items (not just forms) to span the full width, or at least that's what I understood from those comments. In a different discussion, TJ also suggested deprecating hasForm if we moved forward with specifically adding padding for text, which made sense bc we can provide the spacing by default. Not sure if that answers your question.

One thing I did consider, is that rn the modal content is essentially something like this:

.bx--modal-content (16px L/R padding)
├── * (no additional padding)
└── p (20% R padding)

so paragraphs technically have 16px + 20% right padding. I considered doing padding-right: calc(20% - 16px); so that it's a true 20% right padding. Not sure if it makes a big difference. But otherwise, it felt like this solution accounted for all the issues. Do you have another suggestion?

@kodiakhq kodiakhq bot merged commit 2da12d0 into carbon-design-system:main Oct 4, 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
4 participants