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

[link] Support extending theme color with color prop #29596

Open
mark-wang-beep opened this issue Nov 9, 2021 · 4 comments
Open

[link] Support extending theme color with color prop #29596

mark-wang-beep opened this issue Nov 9, 2021 · 4 comments
Labels
component: link This is the name of the generic UI component, not the React module! new feature New feature or request package: material-ui Specific to @mui/material

Comments

@mark-wang-beep
Copy link

mark-wang-beep commented Nov 9, 2021

Summary 💡

Currently Link component's color prop can receive "primary" | "secondary" | "error", for other palette such as warning, a .main postfix is needed, so does custom palette.

The current behavior is shown in a code sandbox.

Expect the .main postfix is not needed.

Examples 🌈

Working code currently:

<Link href="#" color="warning.main">
  Warning
</Link>
<Link href="#" color="foo.main">
  Custom palette
</Link>

Expected behaviour:

<Link href="#" color="warning">
  Warning
</Link>
<Link href="#" color="foo">
  Custom palette
</Link>
@mark-wang-beep mark-wang-beep added the status: waiting for maintainer These issues haven't been looked at yet by a maintainer label Nov 9, 2021
@oliviertassinari oliviertassinari added the component: link This is the name of the generic UI component, not the React module! label Nov 9, 2021
@oliviertassinari
Copy link
Member

oliviertassinari commented Nov 9, 2021

These colors are soft-deprecated:

        <Link href="#" color="primary">
        <Link href="#" color="secondary">
        <Link href="#" color="error">
        <Link href="#" color="warning">

There is already a note about it in the codebase:

https://github.com/mui-org/material-ui/blob/cd79628a7817a9729a97bd9c60ea67a131602b08/packages/mui-material/src/Link/Link.js#L23

It's a legacy from the API of v4. In v5, the color prop of the Link/Typography is actually https://mui.com/system/palette/#color.

I think that it would be great fully rely on the system, and remove all the custom logic in the Link.


Now, there could be another discussion for the system, not the Link.
Should <Box sx={{ color: 'primary' }}> be equivalent to <Box sx={{ color: 'primary.main' }}>?

@oliviertassinari oliviertassinari added breaking change deprecation New deprecation message and removed status: waiting for maintainer These issues haven't been looked at yet by a maintainer labels Nov 9, 2021
@oliviertassinari oliviertassinari added this to the v6 milestone Nov 9, 2021
@oliviertassinari oliviertassinari changed the title [Link] Make color prop support more color from palette [Link] Remove <Link color="warning"> support Nov 9, 2021
@oliviertassinari oliviertassinari changed the title [Link] Remove <Link color="warning"> support [Link] Remove custom <Link color="warning"> support (it should come from the system) Nov 9, 2021
@oliviertassinari oliviertassinari added the package: system Specific to @mui/system label Nov 9, 2021
@mark-wang-beep
Copy link
Author

The color prop also affects the underline's color, so the sx version of <Link href="#" color="warning.main">Warning</Link> is:

<Link
  href="#"
  sx={(theme) => {
    return {
      color: "error.main",
      textDecorationColor: alpha(theme.palette.error.main, 0.4)
    };
  }}
>
  Warning
</Link>

if the alpha channel is 0.4.

@oliviertassinari
Copy link
Member

oliviertassinari commented Nov 10, 2021

The color prop also affects the underline's color

Great point, then maybe we should have the Link's color API match the Button's color API? The color prop is not only about the CSS color property.

If we do such, then I think that we would need to

  1. add the support for 'success', 'error', 'info', 'warning'
  2. remove 'textPrimary', 'textSecondary'?
  3. document these custom colors
    https://github.com/mui-org/material-ui/blob/b0996ed3d0de77b17906570925ffb1d48b83534c/packages/mui-material/src/Link/Link.js#L193
  4. allow TypeScript augmentation

For the Typography, I think that we should remove the custom values as it create confusion/duplication with the system:

https://github.com/mui-org/material-ui/blob/b0996ed3d0de77b17906570925ffb1d48b83534c/packages/mui-material/src/Typography/Typography.js#L76

@oliviertassinari oliviertassinari changed the title [Link] Remove custom <Link color="warning"> support (it should come from the system) [Link] Improve color handling Nov 10, 2021
@oliviertassinari oliviertassinari changed the title [Link] Improve color handling [Link] Improve color prop handling Nov 10, 2021
@aaronadamsCA
Copy link
Contributor

maybe we should have the Link's color API match the Button's color API?

IMO that's the right choice. I just got bit by this and had to step through MUI internals to figure out where I'd gone wrong. <Link color="primary"> works, <Link color="error"> throws TypeError: color.charAt is not a function. This led me to think I'd somehow made a mistake in theme composition, as opposed to passing an unexpected prop value.

@oliviertassinari oliviertassinari changed the title [Link] Improve color prop handling [link] Improve color prop handling Feb 25, 2024
@oliviertassinari oliviertassinari changed the title [link] Improve color prop handling [link] Support extending theme color with color prop Feb 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: link This is the name of the generic UI component, not the React module! new feature New feature or request package: material-ui Specific to @mui/material
Projects
Status: Backlog
Development

No branches or pull requests

4 participants