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

Add removePaddingBottom on <Modal> to remove padding bottom #207

Closed
wants to merge 3 commits into from

Conversation

chenesan
Copy link
Contributor

Purpose

Add removePaddingBottom on <Modal> (in implementation, <ModalContent>) to remove padding-bottom of modal content block. Defaults to false. Also update examples in storybook.

Changes

  • Add removePaddingBotom on <Modal> to remove padding bottom.

UI screenshot

  • Modal with removePaddingBottom=true

Remove the default padding bottom.

螢幕快照 2019-04-24 下午1 58 24

  • Modal with both removePaddingBottom and bodyPadding

The top (not shown in screenshot), left, right of modal will filled with bottom, but bottom padding is still 0.

螢幕快照 2019-04-24 下午1 58 35

Risk

None

TODOs

None

@chenesan chenesan requested review from zhusee2 and tz5514 April 24, 2019 06:01
@chenesan chenesan self-assigned this Apr 24, 2019
Copy link
Contributor

@zhusee2 zhusee2 left a comment

Choose a reason for hiding this comment

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

I was thinking maybe it can be merged into the bodyPadding prop.
When it's true you get a body with both horizontal and bottom padding; and vice versa.

So it's more like if you're implementing a custom layout inside this modal body.
If so, you remove all padding from it.

@zhusee2
Copy link
Contributor

zhusee2 commented Sep 3, 2019

I'm closing this PR in favor for #224, as the later introduced an overhaul of <Modal>'s inner layout.
Thank you for this change 👍

@zhusee2 zhusee2 closed this Sep 3, 2019
@zhusee2 zhusee2 deleted the feature/allow-modal-remove-padding-bottom branch September 3, 2019 09:22
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.

2 participants