[Joy] Button API by siriwatknp · Pull Request #29962 · mui/material-ui

siriwatknp · 2021-11-30T10:07:57Z

This PR will set a standard for the default style of each variant.

Styling APIs

variants (text, outlined, light, contained[default])
colors (primary[default], neutral, danger, info, success, warning)
sizes (sm, md[default], lg)

Others

TS spec
sx prop

I am really not sure about transition property. I think we can postpone the transition discussion in this PR and update later.

I have followed (at least) the PR section of the contributing guide.

mui-pr-bot · 2021-11-30T10:10:19Z

Details of bundle changes

@mui/joy: parsed: -0.47% 😍, gzip: +0.40%

Generated by 🚫 dangerJS against bf364dd

danilo-leal · 2021-12-01T15:37:12Z

Ended up doing some tweaks to other files (the default theme and variantUtils)! I think it would be nice to add to the component preview page what they would like in dark mode. I tried doing it with a dark background but that just doesn't trigger the variants changing to the dark color scheme 😅

siriwatknp · 2021-12-02T08:47:22Z

Ended up doing some tweaks to other files (the default theme and variantUtils)! I think it would be nice to add to the component preview page what they would like in dark mode. I tried doing it with a dark background but that just doesn't trigger the variants changing to the dark color scheme 😅

Added!

docs/pages/experiments/joy/components.tsx

siriwatknp · 2021-12-02T16:10:12Z

packages/mui-joy/src/styles/CssVarsProvider.tsx

 > & {
  breakpoints?: BreakpointsOptions;
  spacing?: SpacingOptions;
+  components?: Components;


add components here to prevent dependency cycle.

mnajdova

From a styling perspective, I am only not sure about the warning color in light mode. It's too green for my eyes :)

packages/mui-joy/src/Button/Button.tsx

docs/pages/experiments/joy/components.tsx

mnajdova · 2021-12-06T09:26:45Z

packages/mui-joy/src/Button/Button.tsx

+   * The border-radius of the component.
+   * @default 'default'
+   */
+  roundness: PropTypes.oneOf(['default', 'lg', 'md', 'sm', 'xl', 'xs']),


I am not sure about default as a value. I remember that at some point when we started on v5, we were actually replacing all default values for the props that we had. As far as I remember, the reasoning was the the default value is not a concrete value, we should rather have it be some specific value, like md. @mbrookes what are your thoughts on this?

Good point, may be the roundness makes sense to not have default because some components can have different values. Ex, Switch might have initial roundness as xl whereas Button has 'md'.

Exactly, and you can probably use the same argument for each prop, the default for one component may not be the same as the default for another, so I wouldn't add it as a key.

Agree that "the default" should be a given size available in the scale (e.g md or sm) instead of default being a plus one value. I think this is a general approach that should be applied to all props, not only border radius, though!

Do we really need a prop for this? Is it common for a UI to have multiple buttons with different border radiuses? It seems like it should be configured through the theme. There might be an argument to be made for a "shape" prop (square, rounded, round), but there's a lot to be said for not adding props prematurely.

The initial intention for having shadow and radius props for all components is for API consistency, so the experience of using is the same across components. However, after discussing with Danilo, it is better not to have these props because:

it encourages dev to make an inconsistent design which is not common for an application to have a different radius for a single component.

we already have sx prop API for one-time customization which can hook to the theme and use the token if needed.

more props, more maintenance afford.

shadow & radius props are removed, please review again.

danilo-leal · 2021-12-06T12:28:30Z

I am only not sure about the warning color in light mode.

Yeah, I'm also finding the yellow-600 on light mode not too great. But this is a tricky one because we're using the 600 shades as bg for the contained variants and if we lighten too much the yellows, the text will have to turn black to keep the contrast ratio which would make this a one-off case. I'll probably iterate a little more on the palette still - adding more colors, grey tones, etc.

siriwatknp · 2021-12-06T15:38:27Z

the text will have to turn black to keep the contrast ratio which would make this a one-off case

I vote for bg yellow with black text. It makes more sense and shows that the variant approach is quite flexible.

packages/mui-joy/src/Button/Button.tsx

mnajdova

Don’t forget to update the warning color 😀

packages/mui-joy/src/Button/Button.spec.tsx

packages/mui-joy/src/Button/Button.tsx

packages/mui-joy/src/Button/ButtonProps.ts

siriwatknp · 2021-12-15T06:31:55Z

test/regressions/README.md

 ### Manual

-`yarn test:regresions:dev` will build all fixtures and render an overview page that lists all fixtures.
+`yarn test:regressions:dev` will build all fixtures and render an overview page that lists all fixtures.


fix typo, nothing else.

siriwatknp · 2021-12-15T06:33:58Z

Don’t forget to update the warning color 😀

Danilo has updated the colors, please check it out in the preview or sandbox. I think the colors should not block the PR in case we are not agreed on the color value.

mbrookes · 2021-12-15T20:18:00Z

I think the colors should not block the PR in case we are not agreed on the color value.

Probably just as well - I've been resisting commenting on the rest of the colors besides blue. 😅

mnajdova · 2021-12-16T10:37:31Z

I think the colors should not block the PR in case we are not agreed on the color value.

But I thought this is the PR where we should discuss it, as it is about the button theme :D Should we look into the structure and API only? Could we then rename the PR title to: "[Joy] Buttom theme structure" or something similar?

mnajdova · 2021-12-16T10:40:11Z

@siriwatknp @danilo-leal should we have clear steps about PR that we should expect when developing a Joy component? For example:

functionality + API
styles + theme

I would even say we should merge these two in one PR, otherwise, it's hard to know what to focus on when reviewing the PR, which means that we may even lose the opportunity to comment on something, just thinking that it is not a scope of the PR.

michaldudak · 2021-12-16T13:57:28Z

One thing I noticed is that on my screen (1.5x scaling) the button content doesn't look centered:

The distance between cap height and the top edge is 23px, but from baseline to the bottom edge there's just 20px. I'm not sure if anything can be done to work around this without affecting 1x and 2x screens, but I'll try to figure something out (I understand it may be harder for you if you don't have access to such a screen).

michaldudak · 2021-12-16T14:21:53Z

packages/mui-joy/src/styles/variantUtils.ts


  containedColor: `#fff`,
-  containedBg: `var(--joy-palette-${color}-500)`,
+  containedBg: `var(--joy-palette-${color}-600)`,


I'm wondering if it wouldn't be better to supply these values to the background property (instead of background-color). It would allow developers to override the theme with gradients (or even images, but I wouldn't consider it a popular use case).

The idea of variant is to be applied globally (for most components). I think in case of gradient or image, it will be specific for component and as always developers can use the same theming API to do that. https://codesandbox.io/s/joy-cra-typescript-forked-be5uj?file=/src/App.tsx

However, if we see a real use case in the future we can also revisit this point.

siriwatknp · 2021-12-21T04:42:45Z

@siriwatknp @danilo-leal should we have clear steps about PR that we should expect when developing a Joy component? For example:

functionality + API

styles + theme

I would even say we should merge these two in one PR, otherwise, it's hard to know what to focus on when reviewing the PR, which means that we may even lose the opportunity to comment on something, just thinking that it is not a scope of the PR.

What if we merge this PR and open another one with "[Joy] theme variant"? because variant is a global style and it is related to colors (I guess this will take a lot of time to refine to get the right one) and we can discuss only about variants (colors). This will give space for other components to continue to be developed on its own, otherwise we will be stuck in this PR.

Now other component PRs does not need to take color into account because it will follow the system.

I would like to merge this PR because I am stuck with a lot of change in this PR which I did not anticipate before. 🙏 eg. default theme structure & types.

danilo-leal · 2021-12-21T04:59:51Z

Spent the last couple of days chewing on @mnajdova's suggestion and I think it makes sense. Especially as Joy components will follow the global styling defined on the theme. Thus, we could try handling the next components PRs separately:

Component API and developer experience: questions like the ones brought up here: "why supporting an elevation prop?"; "sm or small?"; etc.
Visual design: each token/variant specific value and general design iteration.

Each topic can have its rounds of iterations for nailing it down just right but trying to settle down on both at the same time is trickier. They definitely feed off each other but maybe it's better first to agree on a component API design - or at least on an initial draft to play around with - and later refine the visual aspect than the other way around.

So, we could change this pull request title to Button API or something similar?!

siriwatknp · 2021-12-21T10:19:39Z

we could change this pull request title to Button API or something similar?!

Sounds good to me!

In the next component PR, I will try to make the scope smaller and more clear. (Actually, I have tried with the Button already 😂 I hope it will be easier for the next component)

mnajdova

I spotted a few other uses of default in the theme, could you check them? The Button's API itself looks good to me 👍

packages/mui-joy/src/Button/Button.spec.tsx

packages/mui-joy/src/styles/types/typography.ts

mnajdova

I am happy with the changes 👌

…joy/button-theme

siriwatknp added 2 commits November 30, 2021 16:31

add roundness and elevation to Button

bd7aa8c

add joy components

86651e9

update proptypes

b7a62bb

siriwatknp added package: joy-ui Specific to Joy UI. scope: button Changes related to the button. labels Nov 30, 2021

split files to prevent dep cycle

04df07b

siriwatknp force-pushed the joy/button-theme branch from 7f3a4c8 to 04df07b Compare November 30, 2021 11:22

siriwatknp requested a review from danilo-leal December 1, 2021 01:53

some various tweaks

6fee02f

siriwatknp added 3 commits December 2, 2021 14:29

add sx type to button

043b76c

add color scheme toggle

fb641ad

fix ts error

12d5c80

siriwatknp force-pushed the joy/button-theme branch from b24daf9 to 12d5c80 Compare December 2, 2021 08:25

small color adjustment

9aeb19a

siriwatknp commented Dec 2, 2021

View reviewed changes

docs/pages/experiments/joy/components.tsx Show resolved Hide resolved

siriwatknp commented Dec 2, 2021

View reviewed changes

siriwatknp marked this pull request as ready for review December 3, 2021 02:30

siriwatknp requested review from mnajdova and oliviertassinari December 3, 2021 02:30

siriwatknp added 5 commits December 3, 2021 09:34

adjust experiment layout

4ec8f86

add spec test

2345798

add sx spec to Button

6f2d292

simplify demo

f50a433

yarn proptypes

b5f8907

mnajdova reviewed Dec 6, 2021

View reviewed changes

mbrookes reviewed Dec 13, 2021

View reviewed changes

packages/mui-joy/src/Button/Button.tsx Outdated Show resolved Hide resolved

mnajdova reviewed Dec 14, 2021

View reviewed changes

packages/mui-joy/src/Button/Button.spec.tsx Outdated Show resolved Hide resolved

packages/mui-joy/src/Button/Button.tsx Outdated Show resolved Hide resolved

packages/mui-joy/src/Button/ButtonProps.ts Outdated Show resolved Hide resolved

siriwatknp added 5 commits December 15, 2021 10:25

exclude internal slot components for .tsx

3ff1592

remove shadow & radius prop from Button

b115a79

use color as value

d34df54

fix types

6354df2

update button

6f848b2

siriwatknp force-pushed the joy/button-theme branch from d3a59b7 to 6f848b2 Compare December 15, 2021 06:28

siriwatknp commented Dec 15, 2021

View reviewed changes

siriwatknp requested a review from mnajdova December 16, 2021 07:20

michaldudak reviewed Dec 16, 2021

View reviewed changes

siriwatknp changed the title ~~[Joy] Button theme~~ [Joy] Button theme 1st iteration Dec 21, 2021

siriwatknp changed the title ~~[Joy] Button theme 1st iteration~~ [Joy] Button API Dec 21, 2021

mnajdova reviewed Dec 21, 2021

View reviewed changes

siriwatknp added 2 commits December 22, 2021 10:51

remove default token

71f9bb1

replace default token

f7d18da

mnajdova approved these changes Dec 22, 2021

View reviewed changes

siriwatknp added 3 commits December 25, 2021 15:38

Merge branch 'master' of https://github.com/mui-org/material-ui into …

f6fd86a

…joy/button-theme

add public sans to regression test

19bb274

add getThemeVar util

bf364dd

siriwatknp merged commit 1c02cf8 into mui:master Dec 27, 2021

hbjORbj mentioned this pull request Mar 9, 2022

[Joy] Add Avatar component #31303

Merged

1 task

Uh oh!

Conversation

siriwatknp commented Nov 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mui-pr-bot commented Nov 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

danilo-leal commented Dec 1, 2021

Uh oh!

siriwatknp commented Dec 2, 2021

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mnajdova left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mbrookes Dec 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

siriwatknp Dec 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danilo-leal commented Dec 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

siriwatknp commented Dec 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

mnajdova left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

siriwatknp commented Dec 15, 2021

Uh oh!

mbrookes commented Dec 15, 2021

Uh oh!

mnajdova commented Dec 16, 2021

Uh oh!

mnajdova commented Dec 16, 2021

Uh oh!

michaldudak commented Dec 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

siriwatknp commented Dec 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

danilo-leal commented Dec 21, 2021

Uh oh!

siriwatknp commented Dec 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mnajdova left a comment • edited by mbrookes Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

siriwatknp commented Nov 30, 2021 •

edited

Loading

mui-pr-bot commented Nov 30, 2021 •

edited

Loading

mbrookes Dec 8, 2021 •

edited

Loading

siriwatknp Dec 15, 2021 •

edited

Loading

danilo-leal commented Dec 6, 2021 •

edited

Loading

siriwatknp commented Dec 6, 2021 •

edited

Loading

michaldudak commented Dec 16, 2021 •

edited

Loading

siriwatknp commented Dec 21, 2021 •

edited

Loading

siriwatknp commented Dec 21, 2021 •

edited

Loading

mnajdova left a comment •

edited by mbrookes

Loading