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

Add size and weight props to Text #4834

Merged
merged 10 commits into from
Aug 12, 2024
Merged

Add size and weight props to Text #4834

merged 10 commits into from
Aug 12, 2024

Conversation

langermank
Copy link
Contributor

@langermank langermank commented Aug 8, 2024

Extend Text to support size and weight props.

Changelog

New

  • size prop
  • weight prop

Changed

Removed

Rollout strategy

  • Patch release
  • Minor release
  • Major release; if selected, include a written rollout or migration plan
  • None; if selected, include a brief description as to why

Testing & Reviewing

Merge checklist

Copy link

changeset-bot bot commented Aug 8, 2024

🦋 Changeset detected

Latest commit: aa0efb7

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@primer/react Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@github-actions github-actions bot temporarily deployed to storybook-preview-4834 August 8, 2024 23:01 Inactive
Copy link
Contributor

github-actions bot commented Aug 9, 2024

size-limit report 📦

Path Size
packages/react/dist/browser.esm.js 96.2 KB (+0.15% 🔺)
packages/react/dist/browser.umd.js 96.46 KB (+0.18% 🔺)

@github-actions github-actions bot temporarily deployed to storybook-preview-4834 August 9, 2024 19:06 Inactive
@primer primer bot temporarily deployed to github-pages August 9, 2024 20:15 Inactive
@github-actions github-actions bot temporarily deployed to storybook-preview-4834 August 9, 2024 20:15 Inactive
@github-actions github-actions bot temporarily deployed to storybook-preview-4834 August 9, 2024 20:28 Inactive
@langermank langermank marked this pull request as ready for review August 12, 2024 15:45
@langermank langermank requested review from a team as code owners August 12, 2024 15:45
@langermank langermank added this pull request to the merge queue Aug 12, 2024
Merged via the queue into main with commit 7ce1fda Aug 12, 2024
30 checks passed
@langermank langermank deleted the text-props branch August 12, 2024 17:12
@primer primer bot mentioned this pull request Aug 12, 2024
Copy link
Contributor

@iansan5653 iansan5653 left a comment

Choose a reason for hiding this comment

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

Hi! I think the implementation of this PR looks great, but as someone who's been working in the CSS Modules space for the last couple of weeks, I have strong reservations about the concept here.

On the surface it looks like a convenient API, but I am concerned that both usability and maintainability will suffer if we add yet another way to accomplish the same styling task. Primer has long suffered from the challenges of maintaining several different ways to accomplish the same thing -- particularly when it comes to styling.

Having another way to control font size creates ambiguity around the 'correct' approach to change font size. Is it better to use CSS Modules or is it better to use props? How do I keep track of which styling is done via props or via CSS? How do I apply this styling to non-Text elements?

As a simple example, say I need to render a Button with larger text. What's the best way to do that? With CSS Modules as the only option, the answer is obvious and easy to remember:

<Button className={styles.myButton}>...</Button>

Extending Text adds more choices, neither of which are preferable to className but both of which might feel 'more correct' since props are generally preferred to styling:

  • Wrapping the button or its content in another element adds more DOM nodes, which impacts performance: <Button><Text size="large">...</Text></Button>
  • Using as significantly increases code complexity (also, as is provided by styled-components so it may not be supported in the future): <Button as={Text} size="large">...</Button>

For these simple cases like font size, I think we have a lot to gain from universally aligning on CSS Modules as the only available solution. If we take advantage of it, the CSS Modules investment gives us a unique opportunity to make styling fast, simple, easy to learn, and universally consistent.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants