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): add proper right padding to header #13409

Merged

Conversation

jsehull
Copy link
Contributor

@jsehull jsehull commented Mar 27, 2023

Closes #13288

Ensure Heading has proper padding alignment to match design specs shown here
image

Currently, only the paragraph text has the 20% right padding layout

Changelog

New

  • add 20% padding-right for the Header text to match the p text

Changed

  • n/a

Removed

  • n/a

Testing / Reviewing

  1. Open Storybook > Modal > Default (or any version)
  2. Inspect the h3 Header and confirm there is now right padding
    image
  3. Also note that the padding values match the p tag below since they maintain an equal cutoff point (only IF there is a description area on that modal variation)

@jsehull jsehull requested a review from a team as a code owner March 27, 2023 19:29
@netlify
Copy link

netlify bot commented Mar 27, 2023

Deploy Preview for carbon-components-react ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit 2cc726a
🔍 Latest deploy log https://app.netlify.com/sites/carbon-components-react/deploys/6426f3ba5f92b80008c98866
😎 Deploy Preview https://deploy-preview-13409--carbon-components-react.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@netlify
Copy link

netlify bot commented Mar 27, 2023

Deploy Preview for carbon-elements ready!

Name Link
🔨 Latest commit 2cc726a
🔍 Latest deploy log https://app.netlify.com/sites/carbon-elements/deploys/6426f3ba72c4c90008449628
😎 Deploy Preview https://deploy-preview-13409--carbon-elements.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

Copy link
Collaborator

@francinelucca francinelucca left a comment

Choose a reason for hiding this comment

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

Looks like the header is more padded than the content here
image

Should the padding-right be reduced by 3rem since the modal header is padded-right by 3rem?
image

I also think the content is over-padded by 1rem. Since it is right-padded 1rem but then the p tag is reducing 2rem from the 20%
image
image

@jsehull
Copy link
Contributor Author

jsehull commented Mar 28, 2023

@francinelucca Good eye, it does need to account for that right side padding.

Your question had me dig for how the padding-right percentages work currently. Here's what I found when testing a consistent length:


Calculation info
type: (total padding-right)/full div length = right padding % | calculation used
**i.e. padding-right sum of both the item (h3) and outer div
__

h3
current: (99.2 + 3rem)/720 = 20.44% | 20% - 2rem (current as of this PR)
perfect: (96 + 3rem)/720 = 20% | 20% - 2.2rem

p
current: (105.7 + 1rem)/720 = 16.88% | 20% - 2rem
perfect: (128 + 1rem)/720 = 20% | 20% - 0.6rem

The header was close, but the p text area was further off, which is probably why you caught it so fast.


Shall I update them both to use the "perfect 20%" or use a simpler rem calculation? (i.e. p: 20% - 1rem = 19.1%, h3: 20% - 2rem = 20.44%) I didn't know if that makes it easier to understand/maintain.

Copy link
Collaborator

@francinelucca francinelucca 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 that detailed explanation @jsehull. let's just deduct an extra rem in the header and that should be good enough.

packages/styles/scss/components/modal/_modal.scss Outdated Show resolved Hide resolved
Co-authored-by: Francine Lucca <40550942+francinelucca@users.noreply.github.com>
@kodiakhq kodiakhq bot merged commit de8d41b into carbon-design-system:main Mar 31, 2023
@jsehull jsehull deleted the 13288/heading-right-padding branch April 4, 2023 20:56
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.

[Bug]: padding-right of modal-header is different to the guidance
3 participants