Skip to content

RFC: Standardize width props #406

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

Open
levithomason opened this issue Aug 19, 2016 · 11 comments
Open

RFC: Standardize width props #406

levithomason opened this issue Aug 19, 2016 · 11 comments

Comments

@levithomason
Copy link
Member

levithomason commented Aug 19, 2016

Issue

Many components can specify a width in number of grid columns, sometimes also supporting equal width. We've standardized the values we accept (number, string, 'equal').
However, the prop name used to control the width or width of child components is inconsistent.

There are three types of components to consider that implement some type of width prop.

Group Components

The popular vote here was to use widths. Card.Group itemsPerRow was used as an experiment to test it's intuitiveness over widths.

<Card.Group itemsPerRow='2' />    //=> two cards
<Statistic.Group widths='2' />    //=> two statistics
<Button.Group ___='' />          // PR in progress
<Field.Group ___='' />           // PR in progress
<Step.Group ___='' />            // Overlooked, needs PR

Parent Components

Form supports equal width only. Grid.Column parents use columns as it is most intuitive.

<Form widths='equal' />     //=> equal width form
<Grid columns='2' />        //=> two column grid
<Row columns='2' />         //=> two column row

Individual Components

Individually sized components seem to work well using width. This is clear and consistent.

<Column width='2' />    //=> two wide column
<Field width='2' />     //=> two wide field

Heads up, close call API conflict! <Image width='2' /> is applied to the img tag <img width="2" />. ImageGroup however has no concept of column widths at present.

Brainstorm & Analysis

I propose the individual component width prop remain as is. This RFC considers only parent and group width props.

Here are some opinionated but flexible API guidelines:

  1. Intuitive - Users ought to be able to immediately understand the API without explanation
  2. Consistency - Users ought to be able to infer the API of all components after learning only a few
  3. SUI Parity - Departures from SUI conventions should not be taken lightly
  4. Concise - No one wants to use an obtuse API

Plural Names

In use on Parents, not Groups. Here's what it would look like for all components:

<Button.Group buttons='2' />
<Card.Group cards='2' />
<Field.Group fields='2' />
<Statistic.Group statistics='2' />

<Grid columns='2' />
<Row columns='equal' />
<Form fields='equal' />

<Column width='2' />
<Field width='2' />
  1. Intuitive?
    • Groups - Yes - Though, you could argue it may imply "total number" when it is actually "number per row".
    • Parents - Yes - Grid columns is common place. Form fields can be inferred.
  2. Consistent? - Yes - Plural name for groups, plural name of children for parents.
  3. SUI Parity? - Extreme - These map almost exactly to SUI classes:
    • <Button.Group buttons='2' /> == two ui buttons
    • <Grid columns='2' /> == two column grid
    • <Column width='2' /> == two wide column
  4. Concise? - Somewhat - Duplicates Group component name, can be long <Statistic.Group statistics='2' />

width(s)

Previously voted for use on Groups. Here's what it would look like for all components:

<Button.Group widths='2' />
<Card.Group widths='2' />
<Field.Group widths='2' />
<Statistic.Group widths='2' />

<Grid widths='2' />
<Row widths='equal' />
<Form widths='equal' />

<Column width='2' />
<Field width='2' />
  1. Intuitive?
    • Groups - Yes - "Button widths" sets the widths of the buttons.
    • Parents - Somewhat - "Form widths" seems to set widths of forms in a group, but it applies to the field children. This is the case with all the parent components since they contain children of a different name and the width applies to the children (Form>Fields, Grid>Column).
  2. Consistent? - Very - Same word, individual components are singular while all others are plural.
  3. SUI Parity? - No - Only keeps parity on individual components.
  4. Concise? - Very - Always short, never repetitive

itemsPerRow

<Button.Group itemsPerRow='2' />
<Card.Group itemsPerRow='2' />
<Field.Group itemsPerRow='2' />
<Statistic.Group itemsPerRow='2' />

<Grid itemsPerRow='2' />
<Row itemsPerRow='equal' />
<Form itemsPerRow='equal' />

<Column width='2' />
<Field width='2' />
  1. Intuitive?
    • Groups - Very - Clarifies "total number" vs "total per row". Suggests extra items wrap.
    • Parents - Somewhat - Without the child name, it's unclear which "items" will span the row.
  2. Consistent? - Yes - Individual components use a width, while others use a different prop convention.
  3. SUI Parity? - No - Only keeps parity on individual components
  4. Concise? - No - Clear but very verbose

Conclusion & Proposal

With equal requirement weighting plural names are the best pattern overall. This then is also my proposal. Feedback welcome, otherwise any PR closing this issue should implement plural names.

Plural Names (11) width(s) (9) itemsPerRow (7) Category Best
Intuitive Groups 2 2 3 itemsPerRow
Intuitive Parents 2 1 1 Plural Names
Consistent 2 3 3 width(s)
itemsPerRow
SUI Parity 3 0 0 Plural Names
Concise 1 3 0 widths

0 - negative, 1 - ok, 2 - good, 3 - great

@levithomason
Copy link
Member Author

This issue should also consider that the plural name props (fields, cards, etc) are used to pass an array of objects in to generate children. If the plural prop is reserved for widths, there will be a conflict. Whereas, allowing widths also allows passing an array of objects in the intuitive plural name prop:

<Form fields={[...]} widths={3} />

@stale
Copy link

stale bot commented Feb 4, 2018

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 30 days if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Feb 4, 2018
@stale stale bot closed this as completed Mar 6, 2018
@levithomason
Copy link
Member Author

Thanks to @rijk, we have improved how we handle stale issues, see #2761. Reopening.

@levithomason levithomason reopened this May 14, 2018
@stale stale bot removed the stale label May 14, 2018
@brianespinosa
Copy link
Member

FWIW I also just 👍 this. It's currently on the v1 milestone. Might make sense to move this to the v2 milestone instead if this is something that will take a lot to shift to. But I'd prefer the optimization to happen in both from a support standpoint as we move past v1.

@stale
Copy link

stale bot commented Aug 12, 2018

There has been no activity in this thread for 90 days. While we care about every issue and we’d love to see this fixed, the core team’s time is limited so we have to focus our attention on the issues that are most pressing. Therefore, we will likely not be able to get to this one.

However, PRs for this issue will of course be accepted and welcome!

If there is no more activity in the next 90 days, this issue will be closed automatically for housekeeping. To prevent this, simply leave a reply here. Thanks!

@stale stale bot added the stale label Aug 12, 2018
@layershifter
Copy link
Member

Unstale, please :cat

@stale stale bot removed the stale label Sep 17, 2018
@stale
Copy link

stale bot commented Mar 16, 2019

There has been no activity in this thread for 180 days. While we care about every issue and we’d love to see this fixed, the core team’s time is limited so we have to focus our attention on the issues that are most pressing. Therefore, we will likely not be able to get to this one.

However, PRs for this issue will of course be accepted and welcome!

If there is no more activity in the next 180 days, this issue will be closed automatically for housekeeping. To prevent this, simply leave a reply here. Thanks!

@stale stale bot added the stale label Mar 16, 2019
@brianespinosa
Copy link
Member

Unstale 🎬

@stale stale bot removed the stale label Mar 18, 2019
@stale
Copy link

stale bot commented Sep 14, 2019

There has been no activity in this thread for 180 days. While we care about every issue and we’d love to see this fixed, the core team’s time is limited so we have to focus our attention on the issues that are most pressing. Therefore, we will likely not be able to get to this one.

However, PRs for this issue will of course be accepted and welcome!

If there is no more activity in the next 180 days, this issue will be closed automatically for housekeeping. To prevent this, simply leave a reply here. Thanks!

@stale stale bot added the stale label Sep 14, 2019
@Subhojit-Dey1234
Copy link

Subhojit-Dey1234 commented Aug 29, 2021

I am new to open-source contributions and I want to contribute. Can you explain what I have to do? I am a little bit confused.

This issue should also consider that the plural name props (fields, cards, etc) are used to pass an array of objects in to generate children. If the plural prop is reserved for widths, there will be a conflict. Whereas, allowing widths also allows passing an array of objects in the intuitive plural name prop:

<Form fields={[...]} widths={3} />

@Ranzeb
Copy link

Ranzeb commented Aug 23, 2022

I am new to open-source contributions and I want to contribute. Can I handle this issue?

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

No branches or pull requests

5 participants