Skip to content

fix(Modal): updates content spacing#9700

Merged
kodiakhq[bot] merged 4 commits into
carbon-design-system:mainfrom
jnm2377:modal-content-spacing
Oct 4, 2021
Merged

fix(Modal): updates content spacing#9700
kodiakhq[bot] merged 4 commits into
carbon-design-system:mainfrom
jnm2377:modal-content-spacing

Conversation

@jnm2377

@jnm2377 jnm2377 commented Sep 20, 2021

Copy link
Copy Markdown
Contributor

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

netlify Bot commented Sep 20, 2021

Copy link
Copy Markdown

✔️ 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

netlify Bot commented Sep 20, 2021

Copy link
Copy Markdown

✔️ 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

netlify Bot commented Sep 20, 2021

Copy link
Copy Markdown

✔️ 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

@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.

LGTM

@joshblack

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

jnm2377 commented Sep 22, 2021

Copy link
Copy Markdown
Contributor Author

@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