Skip to content

Bugfix: remove Paragraph hardcoded style#1560

Merged
hasparus merged 2 commits intosystem-ui:developfrom
flo-sch:bugfix/remove-paragraph-hardcoded-style
Mar 18, 2021
Merged

Bugfix: remove Paragraph hardcoded style#1560
hasparus merged 2 commits intosystem-ui:developfrom
flo-sch:bugfix/remove-paragraph-hardcoded-style

Conversation

@flo-sch
Copy link
Copy Markdown
Collaborator

@flo-sch flo-sch commented Mar 8, 2021

  • fix(components): remove hardcoded Paragraph style
    • remove hardcoded sx props (such style is easy to opt-in with the theme / variants, but harder to opt-out)

Fixes #1476

Ping @atanasster since you commented on the original PR, just for awareness and potential feedback?

- remove hardcoded sx props (such style is easy to opt-in with the theme / variants, but harder to opt-out)
@flo-sch flo-sch requested a review from atanasster as a code owner March 8, 2021 16:06
@vercel
Copy link
Copy Markdown

vercel Bot commented Mar 8, 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/87h1MaW4hYxQYqA7WVHMgDrQcsia
✅ Preview: https://theme-ui-git-fork-flo-sch-bugfix-remove-paragraph-e01b06.vercel.app

@lachlanjc
Copy link
Copy Markdown
Member

Let's absolutely axe the max width, but I think we should keep the margin: 0 we've currently got. Otherwise, as the source comment indicates, you can use a Theme UI component & end up with non-theme spacing from the user agent styles.

@flo-sch
Copy link
Copy Markdown
Collaborator Author

flo-sch commented Mar 10, 2021

Yeah, also uncertain about the margin...
I'm struggling overall with SoC here, is theme-ui intention to do CSS-reset, or should that be the role of a preset // something else than theme-ui?

In the issue, part of the feedback from @aaronadamsCA is:

I think the margin reset probably belongs in a base theme

But theme-ui does not have a "base theme", that's its goal actually (not to have one, allowing anyone to provide such base style for their UI)
At the same time, it is trivial to opt in AND out of that margin with the default paragraph variant 🤷‍♂️

@hasparus
Copy link
Copy Markdown
Member

you can use a Theme UI component & end up with non-theme spacing from the user agent styles.

Only if you change the variant to one you added, innit? Good to merge from my perspective if I'm not mistaken here.

@hasparus hasparus self-requested a review March 16, 2021 20:40
@hasparus hasparus requested a review from lachlanjc March 16, 2021 20:41
@lachlanjc
Copy link
Copy Markdown
Member

I'm okay to go along with removing the margin, but I don't think it's worth breaking people's designs with this change, especially because I would see it as a bug in Theme UI that components have default margins that are not coming from my theme

@flo-sch
Copy link
Copy Markdown
Collaborator Author

flo-sch commented Mar 16, 2021

Only if you change the variant to one you added, innit?

Not necessarily I think, if you do not inherit a preset setting margin: 0 on that default variant, and we remove it from the component itself (as this PR currently does), then "Paragraph" rendering a "p" by default would inherit the browser's margin

@flo-sch
Copy link
Copy Markdown
Collaborator Author

flo-sch commented Mar 16, 2021

Alright, then let's re-add the margin reset, which can easily be opted out by setting whichever margin value on the text.paragraph default variant :)

- to preserve cross-browser style consistency
@hasparus hasparus merged commit 5296afc into system-ui:develop Mar 18, 2021
@hasparus
Copy link
Copy Markdown
Member

And it's merged! Thanks a lot @flo-sch!

@vanbujm
Copy link
Copy Markdown
Contributor

vanbujm commented Apr 15, 2021

I have been having trouble overriding the margin style.

Can we change:

    <Box
      ref={ref}
      as="p"
      variant="paragraph"
      {...props}
      sx={{
        // reset margin by default: avoid relying on user-agent margins (not aware of theme-ui space scale)
        margin: 0,
        ...sx
      }}
      ...

To:

    <Box
      ref={ref}
      as="p"
      variant="paragraph"
      sx={{
        // reset margin by default: avoid relying on user-agent margins (not aware of theme-ui space scale)
        margin: 0,
        ...sx
      }}
      {...props}
      ...

So that you can use margin props (m, my, mx, etc) to override margin?

@lachlanjc
Copy link
Copy Markdown
Member

Seems fine to me @vanbujm! PR welcome

@vanbujm
Copy link
Copy Markdown
Contributor

vanbujm commented Apr 16, 2021

Seems fine to me @vanbujm! PR welcome

PR is up.

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.

Remove sx props from Paragraph component

4 participants