Skip to content
This repository has been archived by the owner on Jan 20, 2022. It is now read-only.

modal - should be no title OR description on top margin #374

Merged
merged 1 commit into from
Aug 24, 2021

Conversation

cy
Copy link
Contributor

@cy cy commented Aug 24, 2021

I made a mistake in #373, instead of not having a top margin between the content if there's no title and no description, it should be no title OR no description. The mistake's effect can see in this chromatic https://www.chromatic.com/test?appId=5a77a6d38a9e3c002045d20d&id=61255022235bb7003aae37f0

before
image

after
Screen Shot 2021-08-24 at 5 16 41 PM

@cy cy added the bug fix Increment the patch version when merged label Aug 24, 2021
@cy cy requested review from cheapsteak and jgzuke August 24, 2021 21:17
@cy cy self-assigned this Aug 24, 2021
@cy cy added patch Increment the patch version when merged and removed bug fix Increment the patch version when merged labels Aug 24, 2021
Copy link
Contributor

@jgzuke jgzuke left a comment

Choose a reason for hiding this comment

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

One question that might be worth double checking 😄

marginTop:
size === "large" ? 24 : size === "medium" ? 16 : 12,
},
(title || description) && {
Copy link
Contributor

@jgzuke jgzuke Aug 24, 2021

Choose a reason for hiding this comment

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

Im not quite sure about having description here? Does the description render in the top section? I would assume this would just be title &&

Copy link
Member

@cheapsteak cheapsteak Aug 24, 2021

Choose a reason for hiding this comment

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

I think the logic is right but the title is possibly confused?

this makes sense (title || description) ? defaultMargin : noMargin which is what the code has

(gosh this component is getting tangly though, really want to chop it up)

Copy link
Member

Choose a reason for hiding this comment

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

Does the description render in the top section

it looks like it does
image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, the description does render in the top section - very tangly component FOR SURE

Copy link
Member

@cheapsteak cheapsteak left a comment

Choose a reason for hiding this comment

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

🚢

@cy cy merged commit 2e90279 into main Aug 24, 2021
@cy cy deleted the cy/modal-fix-margin-top branch August 24, 2021 21:52
@apollo-bot2
Copy link
Collaborator

🚀 PR was released in v9.6.2 🚀

@apollo-bot2 apollo-bot2 added the released This issue/pull request has been released. label Aug 24, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
patch Increment the patch version when merged released This issue/pull request has been released.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants