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

[Switch] Create SwitchUnstyled and useSwitch #26688

Merged
merged 69 commits into from
Jul 7, 2021

Conversation

michaldudak
Copy link
Member

@michaldudak michaldudak commented Jun 10, 2021

This is an implementation of the SwitchUnstyled component and the useSwitch hook. One chunk of #6218

Summary

  • useSwitch is a low-level utility for both internal and external use when maximum customizability is required. It provides an object to be used as input props
  • SwitchUnstyled creates a structure of components and uses useSwitch internally. Root, Thumb and Input components are overridable via components prop (and component, being equivalent to components.Root).
  • The original Switch component used quite a deep hierarchy: Switch -> SwitchBase -> ButtonBase -> input. The new unstyled one (and, in consequence the Switch) does not use neither ButtonBase nor SwitchBase anymore. Part of the logic was moved to useSwitch (handling of focusVisible) and part was moved to a new useTouchRipple hook.
  • Initially I indented to implment better type checking of componentsProps, but after encountering problems with PropTypes, I created a separate issue for this: [typescript] Better type annotations for unstyled components and componentsProps props #26810
  • A new testing utility has been created: describeConformanceUnstyled. It executes the existing tests that apply to unstyled components + a few new ones (testing components and componentsProps props)

Preview: https://deploy-preview-26688--material-ui.netlify.app/components/switches/#unstyled-switches

To do

  • tests
  • documentation
  • demos
  • minimal styles for SwitchUnstyled -> will be done in a separate PR if needed. For now, the unstyled component is truly unstyled.

@mui-pr-bot
Copy link

mui-pr-bot commented Jun 10, 2021

Details of bundle changes (experimental)

@material-ui/core: parsed: +0.55% , gzip: +0.59%
@material-ui/unstyled: parsed: +9.02% , gzip: +7.55%

Generated by 🚫 dangerJS against 530e91e

Copy link
Member

@mnajdova mnajdova left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Initial review. If we go ahead with this notion of introducing hooks, I think it's interesting to read trough this - https://kentcdodds.com/blog/the-state-reducer-pattern-with-react-hooks

@oliviertassinari
Copy link
Member

Great initiative to open a draft so we can discuss on the changes early. The first thing that got my attention is regarding the constraints. The existing components aren't using the new unstyled ones (yet?). What's the plan regarding this aspect?

I can think of two different approaches:

  1. Unstyled button and unstyled switches are isolated from the existing components. Pros: no risk of integration that fail, more guidance by extracting what works, faster?
  2. We start from scratch, we work on the API regardless of what already exists, and try to make it fit as a second step. Pros: reduce upfront constraints that allows to rethink it, higher quality?

It seems that we are going with option 2.

In the two cases, one big challenge is which level we extract/abstract. It has to be enough abstracted to make it valuable for developers (solve problems for them, otherwise they will write their own hook). It also has to be low level enough to allow us to implement a second design specification

@michaldudak
Copy link
Member Author

Great initiative to open a draft so we can discuss on the changes early. The first thing that got my attention is regarding the constraints. The existing components aren't using the new unstyled ones (yet?). What's the plan regarding this aspect?

My next goal will be changing the Core Switch to use this implementation.

I can think of two different approaches:

  1. Unstyled button and unstyled switches are isolated from the existing components. Pros: no risk of integration that fail, more guidance by extracting what works, faster?
  2. We start from scratch, we work on the API regardless of what already exists, and try to make it fit as a second step. Pros: reduce upfront constraints that allows to rethink it, higher quality?

It seems that we are going with option 2.

I chose to build the components from scratch using the existing functionality as reference. Of course I copied a lot of code from the existing components. I find it easier to start with a blank slate and add things when I see they are necessary, than start from a full-blown component and remove things that are not needed. Additionally, I wanted to build these components in TypeScript from the start.

In the two cases, one big challenge is which level we extract/abstract. It has to be enough abstracted to make it valuable for developers (solve problems for them, otherwise they will write their own hook). It also has to be low level enough to allow us to implement a second design specification

I agree - it's hard to draw a line properly. For sure, all the accessibility features and nothing Material-specific should be in unstyled.
The sooner we start implementing the second theme, the sooner we will validate the decisions made to extracts the unstyled components. Then, we may revisit the unstyled API if necessary.

@oliviertassinari oliviertassinari added the package: base-ui Specific to @mui/base label Jun 14, 2021
@oliviertassinari
Copy link
Member

oliviertassinari commented Jun 14, 2021

My next goal will be changing the Core Switch to use this implementation.

@michaldudak This sounds like a great next step.
I'm not even sure we will need to write dedicated test cases, as we could rely on the end-to-end tests with the integration of Material Design (we have done mostly that so far for the unstyled components).

@mnajdova
Copy link
Member

I'm not even sure we will need to write dedicated test cases, as we could rely on the end-to-end tests with the integration of Material Design (we have done mostly that so far for the unstyled components).

Agree, ideally we should not need to change any tests :)

@michaldudak michaldudak force-pushed the feat/unstyled-switch branch from 2c18d8a to 9ca3348 Compare June 15, 2021 17:17
@michaldudak
Copy link
Member Author

I implemented the Switch component using the useSwitch hook. Looks just like the original and the unit tests pass. Now I'll focus more on making the rest of the tests pass and to improve the docs.

Inspired by #26303 I decided to remove the ButtonBase from the render tree. FocusVisible support is provided by useSwitch and I moved handling the ripple effect to the new useTouchRipple hook. This way, I don't need to include ButtonUnstyled in this PR (I'll move it to another one).

@oliviertassinari
Copy link
Member

oliviertassinari commented Jun 29, 2021

@michaldudak It looks great 👌✨

Capture d’écran 2021-06-29 à 13 09 02

Copy link
Member

@mnajdova mnajdova left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks pretty close to be ready for merge :)

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Jun 30, 2021
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Jul 1, 2021
@michaldudak
Copy link
Member Author

@oliviertassinari, @mnajdova I addressed all the comments you made. Could you please take a final look at the PR and see if there's anything outstanding from your point of view?

} = props;

const Root: React.ElementType = component ?? components.Root ?? 'span';
const rootProps: any = { ...getForwardableProps(Root, otherProps), ...componentsProps.root };
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this not enough?

Suggested change
const rootProps: any = { ...getForwardableProps(Root, otherProps), ...componentsProps.root };
const rootProps: any = { ...otherProps, ...componentsProps.root };

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If Root is a plain HTML element, I don't want to forward anything that doesn't apply to a HTML element.

Copy link
Member

@oliviertassinari oliviertassinari Jul 5, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean, I don't get why "foo" shouldn't warn when it does with the buttons.

import * as React from "react";
import Button from "@material-ui/core/Button";
import SwitchUnstyled from "@material-ui/unstyled/SwitchUnstyled";

export default function UnstyledSwitches() {
  return (
    <div>
      <SwitchUnstyled foo />
      <Button foo />
      <button foo />
    </div>
  );
}

https://codesandbox.io/s/unstyledswitches-material-demo-forked-rsrd1?file=/demo.tsx

The developers can swallow the extra prop. The new logic only check it with the "other" props on the root element, but not with the componentsProps slots.

We didn't have the logic up until, it didn't seem to be a pain point. What use case do you have in mind?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My thinking was that forwarding props from the host to the root is less explicit than just setting them in componentsProps.root and therefore it may need some additional checking. If someone explicitly passes something in componentsProps.*, I assume they know what they are doing.

This has an inconvenience of being inconsistent, though. I'm not particularly attached to this piece of code and if you feel we should always forward everything, so be it.

Copy link
Member

@oliviertassinari oliviertassinari Jul 6, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cc @mui-org/core for more perspectives on the matter.

Outside of this point, the PR is good to go, from my side.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would forward all props, as it was done before. Let's not complicate the code for problems we haven't encounter so far.

Copy link
Member

@mnajdova mnajdova left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One tiny last comment for improving the demo. Let's also resolve #26688 (comment) before merging. Well done 🚀

Comment on lines 250 to 260
const SwitchThumb = ({ isChecked, icon, checkedIcon, className }) => {
if (!isChecked && icon) {
return icon;
}

if (isChecked && checkedIcon) {
return checkedIcon;
}

return <DefaultSwitchThumb className={className} />;
};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
const SwitchThumb = ({ isChecked, icon, checkedIcon, className }) => {
if (!isChecked && icon) {
return icon;
}
if (isChecked && checkedIcon) {
return checkedIcon;
}
return <DefaultSwitchThumb className={className} />;
};
const SwitchThumb = styled(({ isChecked, icon, checkedIcon, className }) => {
if (!isChecked && icon) {
return icon;
}
if (isChecked && checkedIcon) {
return checkedIcon;
}
return <span className={className} />;
}, {
name: 'MuiSwitch',
slot: 'Thumb',
overridesResolver: (props, styles) => styles.thumb,
})(({ theme }) => ({
boxShadow: theme.shadows[1],
backgroundColor: 'currentColor',
width: 20,
height: 20,
borderRadius: '50%',
}));

How about we merge this component with the DefaultSwitchThumb? We would avoid having to think about names for components :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't equivalent. icon and checkedIcon shouldn't be styled by us.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you mean? They are not, they would be styled if we add the className prop no?

Copy link
Member Author

@michaldudak michaldudak Jul 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In your suggestion, if icon or checkedIcon is provided, they would have the styles applied (boxShadow, backgroundColor, etc.), which is not what we want Sorry, I made wrong assumption about how styled works. You're right. I'll correct it.

@oliviertassinari oliviertassinari changed the title [SwitchUnstyled] Create SwitchUnstyled and useSwitch [Switch] Create SwitchUnstyled and useSwitch Jul 6, 2021
@michaldudak michaldudak merged commit 0230df9 into mui:next Jul 7, 2021
@michaldudak michaldudak deleted the feat/unstyled-switch branch July 7, 2021 10:05
@oliviertassinari oliviertassinari mentioned this pull request Jul 11, 2021
@khushalsrashtasoft
Copy link

khushalsrashtasoft commented Apr 17, 2024

image

@mui-pr-bot use i am import { Switch } from "@mui/material";

I have this issue. I don't know what issue. above image switch casse css reflet but When I refresh page it gets fixed

@michaldudak
Copy link
Member Author

@khushalsrashtasoft if you're experiencing problems with the Switch, please open a new issue and provide reproduction steps.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: switch This is the name of the generic UI component, not the React module! package: base-ui Specific to @mui/base
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

7 participants