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

[RFC] Renaming Joy APIs #32141

Closed
siriwatknp opened this issue Apr 6, 2022 · 8 comments
Closed

[RFC] Renaming Joy APIs #32141

siriwatknp opened this issue Apr 6, 2022 · 8 comments
Labels
discussion package: joy-ui Specific to @mui/joy RFC Request For Comments

Comments

@siriwatknp
Copy link
Member

siriwatknp commented Apr 6, 2022

This RFC list several points related to renaming Joy UI APIs so that we can discussed before making the changes.

  1. Rename color prop to palette Decided to go with textColor prop instead, details here:

    <Link color="primary" textColor="#000">
  2. ✅ Rename variant values: The reason to rename the values is to set apart from these existing names (CSS properties, color modes) and make them meaningful as much as possible. [Joy] Rename variants #32489

    These are other names that we can consider.

    • bordered (but it is too close to CSS property border)
    • ghost (similar to Chakra UI)
    • filled, subtle (from Mantine)
  3. ~Rename primary palette to brand: continue from [WIP] Fluent button and switch light theme #31090 (comment) to gather more thoughts. ~

  4. Add/Remove typography scale: After doing POC with several apps, it becomes clear that the current h1 and h2 are too big and I have never used h5, h6 (as an HTML tag). I have to always do this <Typography level="h2" fontSize="lg" />. It makes more sense to have display1, 2 for the headline like in MUI branding.

    • ✅ add display1 and display2
    • remove h5 and h6

    From tailwind CSS https://play.tailwindcss.com/uj1vGACRJA?layout=preview
    We haven't used an h4 yet
    But now we have. Please don't use h5 or h6 in your content, Medium only supports two heading levels for a reason, you animals. I honestly considered using a before pseudo-element to scream at you if you use an h5 or h6.

We don't style them at all out of the box because h4 elements are already so small that they are the same size as the body copy. What are we supposed to do with an h5, make it smaller than the body copy? No thanks.

  1. Rename level prop on the typography component to scale for consistency with the system: level was initially added in Joy to replace the variant (because Joy's variant is different). Some discussion is in https://www.notion.so/mui-org/MUI-System-890bb3d81cbf45008fd5d2f90f53539d#5de65bde31ec4a84931cd62081d27e04

If you think other APIs need to be renamed, feel free to add it to the list. Not every bullets have to be decided in this RFC, we can start working on the first batch and revisit the rest later.

@siriwatknp siriwatknp added the package: joy-ui Specific to @mui/joy label Apr 6, 2022
@danilo-leal
Copy link
Contributor

danilo-leal commented Apr 6, 2022

Thanks for writing this up, Jun! Here's my perspective:

  1. 👍 All good with me! Agree that using palette avoids confusion as you can be using several different shades for different states and only "color" might not put this across as clearly as it could. Aside from the confusion with CSS color property.
  2. About variant values:
    • 👍 for solid, instead of contained.
    • 👍 for soft instead of light.
    • Really don't think that using outlined would overlap with outline from CSS, even though it would be good to respect the proposed heuristic. I think that this is a case where relying on the most common is ok, outlined sounds fine to me! These other options just don't come intuitively I guess.
    • 👍 for ghost instead of text (there are a few more options for this one we could consider, i.e flat, invisible, minimal—I lean towards more to ghost or plain)
  3. I believe both would work. You can argue that your primary color is most often (if not always) your main brand color, so... Regardless, I think it's a matter of how much using convention would be more effective. Primary seems to be way more common than brand. Would using brand then impact negatively any dev and/or designer using Joy? I'd bet that no. But either way, I guess I'd not change it just to avoid unnecessary confusion if it happens.
  4. I agree! But I'd do this one separately from the other renaming tasks—as it is not a purely renaming task also. It's more of an opportunity to revisit the 1st typographic scale we came up with and I would love to do that now that we're maturing things a bit more.
  5. 👍 All good for me! Scale indeed sounds better when talking about typography.

@oliviertassinari
Copy link
Member

oliviertassinari commented Apr 14, 2022

  1. It sounds like a good candidate for Material UI v6 too.

too close to the CSS outline property

When was the last time you used this CSS property? I almost forgot it existed 😁

  1. On the HTML element: https://www.w3.org/TR/2011/WD-html5-author-20110809/the-h1-h2-h3-h4-h5-and-h6-elements.html uses "rank" to compare h1 to h2, h3, etc. https://developer.mozilla.org/en-US/docs/Web/HTML/Element/Heading_Elements uses "level".
    On the design aspect https://m3.material.io/styles/typography/tokens uses "scale" to descript the concept.

@siriwatknp
Copy link
Member Author

siriwatknp commented Apr 15, 2022

When was the last time you used this CSS property? I almost forgot it existed 😁

Joy use outline as a default for focus visible because you can use CSS outlineOffset to control the inward/outward of the outline and it does not overlap with CSS border/box-shadow that people usually customize.

Another benefit is that in Chrome & Firefox (too sad for Safari), the outline radius follows the border-radius value.

@danilo-leal
Copy link
Contributor

I'd say that using outlined, in the past tense, is enough to avoid confusion with CSS's outline. It's widely used and following convention here seems like a safe thing to do.

@mnajdova
Copy link
Member

  1. Rename color prop to palette

I am not sure only on this one. I think the problem is not with the color prop itself, but how we use the same term for different things. In my opinion, we should keep the API close to how you would describe the UI if you see it. For example if I see a button with blue background I would say that the button has a blue color, not that it uses the blue palette. The problem is with the CSS properties that we use on some of the components as props. If I go one step further, I would say that the css prop color is a bad chosen name (I am being wild here :D), it should be something like textColor, similar to backgroundColor. My proposal would be renaming the CSS prop we use on the components from color to textColor. This way people won't confus this with the color prop that exists on other components (or on the same component as in the case of the Link component).

@samuelsycamore
Copy link
Member

My proposal would be renaming the CSS prop we use on the components from color to textColor.

+1 for this.

@siriwatknp
Copy link
Member Author

My proposal would be renaming the CSS prop we use on the components from color to textColor. This way people won't confus this with the color prop that exists on other components (or on the same component as in the case of the Link component)

Interesting choice! I listed the efforts to help us decide.

1. Add textColor prop (NOT a breaking change ☺️)

  • Update extendSxProp to convert textColor prop
  • Add textColor prop to Typography, Grid, Stack
  • Fix the Link to use textColor prop
  • Clarify the meaning of color & textColor prop in the docs

2. Renaming to palette prop (BREAKING CHANGE)

  • Rename every component (tests, classes, types, implementation) from color to palette
  • Create a codemod (prop, className)
  • Clarify the meaning of palette prop in the docs

For me, I'll vote for @mnajdova option because it is not a breaking change and require a lot smaller effort. We have to write a doc to clarify the meaning of the prop regardless of the choice.

@danilo-leal
Copy link
Contributor

Sweet! Let's go for textColor then, it indeed seems like a better alternative!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion package: joy-ui Specific to @mui/joy RFC Request For Comments
Projects
None yet
Development

No branches or pull requests

5 participants