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: Allow overriding Paragraph margins via theme #1775

Merged
merged 1 commit into from
May 26, 2021
Merged

fix: Allow overriding Paragraph margins via theme #1775

merged 1 commit into from
May 26, 2021

Conversation

bernharduw
Copy link
Contributor

@bernharduw bernharduw commented May 25, 2021

This fixes #1774 - I found that margin: 0 is set by default (coming from Box maybe? Not 100% sure).

So if we don't set the margin for Paragraph manually, the specificity issues disappear.

I also added two tests using a theme override - the other two cases are already covered by the existing tests.

Version

Published prerelease version: v0.9.1-develop.0

Changelog

🐛 Bug Fix

  • @theme-ui/components

Authors: 1

@vercel
Copy link

vercel bot commented May 25, 2021

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/systemui/theme-ui/47JD5Y5pEcvNmyvyW9autuBcwSij
✅ Preview: https://theme-ui-git-fork-bernharduw-fix-paragraph-theme-m-41f60d.vercel.app

@hasparus
Copy link
Member

Thanks for the PR @bernharduw!

@hasparus
Copy link
Member

hasparus commented May 25, 2021

Doesn't this break the scenario when users depend on margin being set to 0 by default? (See snapshot in the diff.)

@@ -1097,7 +1097,6 @@ exports[`Paragraph renders 1`] = `
font-family: body;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The snapshot still has margin: 0 in it, but at the very beginning.

@bernharduw
Copy link
Contributor Author

I think that way the effective margin on paragraphs is still 0, but can be overridden by any styling method, regardless of theme, styled-system-style or direct sx overrides. The tests still all pass, only the duplicate margin: 0 is reduced to a single occurrence.

The margin:0 in line 1095 comes from the Box which sets margin:0 as a default, so we should be safe here.

@hasparus
Copy link
Member

Thanks! Yup, I can see that first margin: 0. Sorry!

@hasparus hasparus merged commit 06857c0 into system-ui:develop May 26, 2021
@hasparus hasparus added the prerelease This change is available in a prerelease. label May 26, 2021
@bernharduw bernharduw deleted the fix-paragraph-theme-margins branch May 26, 2021 09:00
@hasparus hasparus added released This issue/pull request has been released. and removed prerelease This change is available in a prerelease. labels May 26, 2021
@hasparus
Copy link
Member

🚀 PR was released in v0.9.2-develop.0 🚀

@hasparus hasparus added the prerelease This change is available in a prerelease. label May 27, 2021
@sergeylukin
Copy link

WD for removing duplication. I, however, struggle to understand the purpose of Box's default margin: 0... which causes troubles when parent intends to set inner Boxes' margin via '> * + *': 'marginLeft: 30px' for instance.
Why do we need to set margin: 0? If it's inherited, we can set m={0} in case it's required (which is an edge case really).
Would love to know more about that. Thanks!
Hope that's not too far from the subject lol

@lachlanjc
Copy link
Member

@sergeylukin I believe it's just a reset, since elements like form fields & lists often have margin applied by the default browser stylesheet. Happy to accept better solutions to that problem, though!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
prerelease This change is available in a prerelease. released This issue/pull request has been released.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Paragraph margin can't be overridden by theme
4 participants