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

Components: Audit use of size prop across components #42792

Open
mirka opened this issue Jul 29, 2022 · 1 comment
Open

Components: Audit use of size prop across components #42792

mirka opened this issue Jul 29, 2022 · 1 comment
Labels
[Package] Components /packages/components [Type] Enhancement A suggestion for improvement.

Comments

@mirka
Copy link
Member

mirka commented Jul 29, 2022

What problem does this address?

Some components have a size prop that behave in slightly different ways. This may lead to confusion and bugs.

What is your proposed solution?

Audit the usage of the size prop across components, and see if there are safeguards we can put in to prevent confusion and mistakes.

Known components

  • Control components (InputControl, SelectControl, etc.) have a size prop to control height variants.
  • Card: Has a size prop to control padding sizes. The prop name is ambiguous, maybe rename to paddingSize?
  • Text: Has a size prop to control font-size. The prop name is appropriate, but the complexity here is that it accepts either a number (interpreted as a ratio of base font size), preset enum string, or CSS value string. Maybe we should simplify. (Acutal bug that could've been prevented with TS if the accepted values were simpler InputControl: Fix incorrect size prop passing to Text #42793)
@mirka mirka added the [Package] Components /packages/components label Jul 29, 2022
@mirka mirka added this to Inbox (needs triage) 📬 in WordPress Components via automation Jul 29, 2022
@ciampo
Copy link
Contributor

ciampo commented Jul 29, 2022

Brainstorming a few ideas.

  • maybe we should never have a prop called just size, and instead be a bit specific with the prop name ? I.e. size as intended for control components could be renamed to sizeVariant, size on the Text component should be called fontSize, and size on the Card component could be renamed as you suggested to paddingSizeVariant (although, as you know, I'm never a fan of exposing "implementation details" such as "padding" in a prop name)
  • More in general, I like the idea that props that allow only a fixed list of preset values (e.g. small, medium, large) have variant added as a suffix

@jordesign jordesign added the [Type] Enhancement A suggestion for improvement. label Sep 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] Components /packages/components [Type] Enhancement A suggestion for improvement.
Projects
WordPress Components
Inbox (needs triage) 📬
Development

No branches or pull requests

3 participants