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

[Fab] Rename round -> circular for consistency #21903

Merged
merged 10 commits into from
Aug 1, 2020

Conversation

kodai3
Copy link
Contributor

@kodai3 kodai3 commented Jul 24, 2020

Breaking change

  • Rename round to circular for consistency. The possible values should be adjectives, not nouns:

    -<Fab variant="round">
    +<Fab variant="circular">
  • I have followed (at least) the PR section of the contributing guide.

[Fab] Rename Fab round -> circle for consistency

I thought we should do the same on Pagination and PaginationItem

Part of #21964

@mui-pr-bot
Copy link

mui-pr-bot commented Jul 24, 2020

Details of bundle changes

Generated by 🚫 dangerJS against f9615a8

@oliviertassinari oliviertassinari changed the title [Fab]Rename round -> circle for consistency [Fab] Rename round -> circle for consistency Jul 24, 2020
@oliviertassinari oliviertassinari added the component: Fab The React component. label Jul 24, 2020
@oliviertassinari
Copy link
Member

oliviertassinari commented Jul 24, 2020

@kodai3 Thanks for working on it, the latest discussion we had on the topic with @mbrookes is #18116 (comment). A benchmark:

  • Pagination (& Item): shape: PropTypes.oneOf(['round', 'rounded']),
  • Fab: variant: PropTypes.oneOf(['extended', 'round']),
  • Avatar: variant: PropTypes.oneOf(['circle', 'rounded', 'square']),
  • Badge: overlap: PropTypes.oneOf(['circle', 'rectangle']),
  • Skeleton: variant: PropTypes.oneOf(['circle', 'rect', 'text']),

If we look at the semantic of circle vs round:

  • Circle is a noun describing a very precisely defined 2-D geometric shape that has a center and a radius and every point on the circle has a defined relationship with the center.
  • Round is an adjective. Very loose.
  1. shaped like or approximately like a circle or cylinder."she was seated at a small, round table"synonyms:circular, ring-shaped, disk-shaped, hoop-shaped; More
  2. shaped like or approximately like a sphere."a round glass ball"

I would vote for circle so we stay close to the geometric semantic field we have with square and rectangle. It's an accurate representation of the geometric shape of the component. It also minimizes BCs.


Also, check how we should harmonize rectangle vs rect. I would vote for the former.

@kodai3
Copy link
Contributor Author

kodai3 commented Jul 24, 2020

I totally agree with you.
addition to that, having similar spell like 'round' | 'rounded', felt unintuitive at least for me.

@kodai3
Copy link
Contributor Author

kodai3 commented Jul 24, 2020

Also, check how we should harmonize rectangle vs rect. I would vote for the former.

i would do for the former too👍

@oliviertassinari
Copy link
Member

Cool, let's wait to get more feedback before moving in any direction.

@mbrookes
Copy link
Member

mbrookes commented Jul 24, 2020

circle [is] an accurate representation of the geometric shape

A Fab or Button as a noun would be described by the adjective "round" (or "circular"), "rounded", "rectangular", "square"* etc.

A square button.
A round button / A circular button.
A rounded button.
A rectangular button.

A circle is round (or circular), but a round button isn't a circle. (Perhaps, if it has an outline; but it still sounds odd to say "a circle button".

That said, we've allowed "rect", "rectangle" and "circle", so if the vote is for consistency over correctness, in order to minimise breaking changes, I'm not going to argue.


*"Square" is a bit of an anomaly here, as the noun and adjective are the same word.

@oliviertassinari
Copy link
Member

oliviertassinari commented Jul 24, 2020

I like the idea to use the adjective over the noun, it seems more consistent and accurate.

@oliviertassinari
Copy link
Member

So?

- Pagination (& Item): `shape: PropTypes.oneOf(['round', 'rounded']),`
- Fab: `variant: PropTypes.oneOf(['extended', 'round']),`
- Avatar: `variant: PropTypes.oneOf(['circle', 'rounded', 'square']),`
- Badge: `overlap: PropTypes.oneOf(['circle', 'rectangle']),`
- Skeleton: `variant: PropTypes.oneOf(['circle', 'rect', 'text']),`
+ Pagination (& Item): `shape: PropTypes.oneOf(['circular', 'rounded']),`
+ Fab: `variant: PropTypes.oneOf(['extended', 'circular']),`
+ Avatar: `variant: PropTypes.oneOf(['circular', 'rounded', 'square']),`
+ Badge: `overlap: PropTypes.oneOf(['circular', 'rectangular']),`
+ Skeleton: `variant: PropTypes.oneOf(['circular', 'rectangular', 'text']),`

@kodai3
Copy link
Contributor Author

kodai3 commented Jul 27, 2020

@oliviertassinari Do you think it is ok to step forward with #21903 (comment) ?

or maybe should we create a dedicated issue to get more feedback?

@oliviertassinari
Copy link
Member

I think that we should wait @mbrookes feedback first. It's not a small change.

@mbrookes
Copy link
Member

The API LGTM, but I agree it would be good to get broader support for this change before diving into a PR. @kodai3 do you want to create an RFC issue?

@kodai3
Copy link
Contributor Author

kodai3 commented Jul 27, 2020

@oliviertassinari @mbrookes created the issue.
Thanks!

@github-actions github-actions bot added PR: out-of-date The pull request has merge conflicts and can't be merged and removed PR: out-of-date The pull request has merge conflicts and can't be merged labels Jul 28, 2020
@kodai3
Copy link
Contributor Author

kodai3 commented Aug 1, 2020

@oliviertassinari @mbrookes changed to circular, should be ready for review

@eps1lon eps1lon changed the title [Fab] Rename round -> circle for consistency [Fab] Rename round -> circular for consistency Aug 1, 2020
@oliviertassinari oliviertassinari merged commit de2af45 into mui:next Aug 1, 2020
@kodai3 kodai3 deleted the feat/fab-rename-property branch August 2, 2020 22:08
@eps1lon eps1lon added this to the v5 milestone Sep 15, 2020
@oliviertassinari oliviertassinari mentioned this pull request Sep 15, 2020
42 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants