Skip to content

Conversation

@mitchmaps
Copy link
Contributor

Why are these changes introduced?

Fixes #2061

What issue is this PR addressing?

The Modal component currently has an inconsistent top and bottom spacing in the section subcomponent.

What is this pull request doing?

This very simple change removes a repeated function call that made the top padding set to the base Polaris token value rather than the correct loose one. Now the top and bottom padding for the section is set to loose.

Before

image
image

After

image
image

@ghost
Copy link

ghost commented Sep 3, 2019

👋 Thanks for opening your first pull request. A contributor should give feedback soon. If you haven’t already, please check out the contributing guidelines.

Copy link
Contributor

@dleroux dleroux 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 doing this @mitchmaps ! Although I think the issue might have not been correct. The padding is correct on the .section, the issue is that there is no reason to have different rules for :first-of-type. Unless @AdamWhitcroft thinks it should be the be same all around (top right bottom left), we simply need to remove the :first-of-type.

@kaelig
Copy link
Contributor

kaelig commented Sep 3, 2019

Can this also be fixed in polaris-rails, and perhaps Shopify/shopify for consistency?

@AdamWhitcroft
Copy link

Hey @dleroux, not super sure I follow what you mean, but from my POV the After screenshot from above here is what I'd like. The content in the modal sits within uniform padding from top/bottom/left/right of the section.

@dleroux
Copy link
Contributor

dleroux commented Sep 5, 2019

Ok perfect. Just to clarify, the Sections are normally 16px top and bottom and 20px for the sides. For some unknown reason, the first section only has 20px at the top. This PR should have all sections including the first one to have 20px all around.

padding: spacing() spacing(loose);
padding: spacing(loose);

&:first-of-type {
Copy link
Contributor

Choose a reason for hiding this comment

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

🔥

@mitchmaps mitchmaps force-pushed the update-modal-spacing branch 2 times, most recently from 0cb248a to d8254d3 Compare September 6, 2019 14:28
@mitchmaps mitchmaps requested a review from dleroux September 6, 2019 14:31
@mitchmaps mitchmaps force-pushed the update-modal-spacing branch from 7cdb40e to 0f5a704 Compare September 6, 2019 14:33
@dleroux
Copy link
Contributor

dleroux commented Sep 6, 2019

Thanks for doing this @mitchmaps! 🐑 🇮🇹 🎉

Following @kaelig comment is this something you could tackle in rails?

@mitchmaps
Copy link
Contributor Author

No problem @dleroux ! And yes I'll hop into that on Monday!

@mitchmaps
Copy link
Contributor Author

mitchmaps commented Sep 9, 2019

@dleroux looks like this change has already been implemented in polaris-rails upon inspection :)

Screen Shot 2019-09-09 at 9 54 24 AM

@mitchmaps mitchmaps merged commit d1342b2 into master Sep 9, 2019
@mitchmaps mitchmaps deleted the update-modal-spacing branch September 9, 2019 13:47
@chloerice chloerice temporarily deployed to production September 9, 2019 21:32 Inactive
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Update modal sections to have uniform top and bottom spacing

5 participants