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

feat: paragraph component #1298

Merged
merged 6 commits into from
Nov 29, 2020
Merged

feat: paragraph component #1298

merged 6 commits into from
Nov 29, 2020

Conversation

flo-sch
Copy link
Collaborator

@flo-sch flo-sch commented Nov 25, 2020

Relates to #1250

Add a new <Paragraph /> component, defaulting to HTML <p> element (getting the semantics).
Also add a new doc page for it.

@flo-sch flo-sch changed the title Feature/paragraph Feat: Paragraph component Nov 25, 2020
@flo-sch flo-sch changed the title Feat: Paragraph component feat: paragraph component Nov 25, 2020
@flo-sch
Copy link
Collaborator Author

flo-sch commented Nov 25, 2020

First try, let me know what you guys think? @atanasster @isaac-martin @hasparus

Few notes:

  • <Text /> remains unchanged, since converting it to a span would be a BC-breaking change, potentially for a separate PR, potentially later (not sure which release this would be included in)?
  • No different themeKey yet, since <Heading /> does not have one neither, I am wondering about whether this would be confusing?
  • Not too sure about the default __css neither, I was inspired by Heading for those but cannot say whether it is good or not to use them here?


# Paragraph

Primitive typographic component for text blocks, defaults to `<p>`, and without margin.
Copy link
Member

Choose a reason for hiding this comment

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

Nitpick. There's something not right with this sentence.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Something like my frenglish, perhaps? 😁

Copy link
Member

@hasparus hasparus left a comment

Choose a reason for hiding this comment

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

LGTM, I'll wait for second opinion for a bit, but it's okay to merge I think

@atanasster
Copy link
Collaborator

Looks great - just a few small items

  • I would suggest to have a separate theme key (variant) paragraph for <Paragraph /> inside the text space
  • I have seen Paragraph in some libs to have a default max-width, or default margin-bottom values (tailwind-css, grommet). We can keep for now away from such defaults, but having a separate theme variant key would allow to separate styling from the <Text />

@hasparus
Copy link
Member

hasparus commented Nov 25, 2020

I have seen Paragraph in some libs to have a default max-width, or default margin-bottom values (tailwind-css, grommet). We can keep for now away from such defaults, but having a separate theme variant key would allow to separate styling from the

Yes, please. Default max-width. We can set something simple there like 760px to avoid resize on font load, but let's set it to set up a precedent. If somebody want full page wide paragaphs, they can suffer.

We use the last value from sizes scale, multiply it by two, whatever, but I'm strongly for setting that max-width.

@flo-sch
Copy link
Collaborator Author

flo-sch commented Nov 25, 2020

Alright, the reason for using the same themeKey is the consistency with Heading: why would heading variants be based on theme.text when paragraphs are separated?

But if you think it is better, I will add it

@atanasster
Copy link
Collaborator

@flo-sch - sorry, I meant the variant (default vs paragraph) - and I called it incorrectly theme key, but not the _themeKey prop. Exactly like Heading.

@flo-sch
Copy link
Collaborator Author

flo-sch commented Nov 26, 2020

Ah alright, yes no problem with a default paragraph variant, that makes sense :)

@atanasster
Copy link
Collaborator

Tailwindcss also resets margins to 0 for paragraph (i think it comes from normalizecss)

@flo-sch
Copy link
Collaborator Author

flo-sch commented Nov 26, 2020

Box already does that, that's why I did not, but perhaps it is good not to rely on inherited here?

@atanasster
Copy link
Collaborator

I think users are complaining Box does it, since its supposed to be non-opinionated.

Its up to you though, i am just drafting suggestions as they come to mind. Your PR looks fantastic in any way.

- set margin: 0
- set maxWidth
- update snapshots
- mention default 'paragraph' variant
@flo-sch
Copy link
Collaborator Author

flo-sch commented Nov 27, 2020

I have added those defaults as part of sx props to make sure anyone can override them (if they want to), also with a hardcoded MQ (since I thought it is better not to rely on breakpoints...? Not too sure myself about that anyway, let me know what you think?)

@atanasster
Copy link
Collaborator

excellent, lgtm

@flo-sch
Copy link
Collaborator Author

flo-sch commented Nov 27, 2020

Sweat :)

I have no clue how the release works here, so I would rather have someone knowing it merging the PR whenever they think it's time

Do not hesitate if you feel something should be added?

@hasparus hasparus merged commit c592b6f into system-ui:master Nov 29, 2020
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.

3 participants