mui / material-ui

Material UI: Comprehensive React component library that implements Google's Material Design. Free forever.
https://mui.com/material-ui/
MIT License
93.74k stars 32.24k forks source link

[ButtonBase] Inconsistent ripple style with the material specs #19664

Open arsen opened 4 years ago

arsen commented 4 years ago

If you compare the ripple effect from material specs, it feels like the one here is a bit slower than the one at Google, animation duration should be a little shorter.

You can see it yourself.

mbrookes commented 4 years ago

MCW isn't the spec, it's another Google team's interpretation of it, although ironically material.io now uses MCW for their "Interactive demo" giving the sense that it is definitive, when often it deviates from the spec.

Could you reference the spec at material.io (but not an "interactive demo" 🙂)?

Studio384 commented 4 years ago

The List specs show the ripple effect in various demos that are part of the specs and not another interpretation by a Google team. They do seem faster than the ripple effect that MUI uses today.

mbrookes commented 4 years ago

It's a shame the ripple doesn't have specific guidelines, as it seems to vary from one component to another, depending on the designer who created the video.

I can't work out why from the code, but comparing the ripple on the Card component with that on the List, it appears that the Card ripple is much faster, despite the two components being the same width.

I wondered if the speed was in some way proportional to the area, but DURATION is a fixed value. 🤷‍♂

arsen commented 4 years ago

I think that it should at least be configurable, the speed of the ripple effect. Looks like the duration is a fixed const.

https://github.com/mui-org/material-ui/blob/c06a88d24235685dd769c1a3df82152ded17a6ca/packages/material-ui/src/ButtonBase/TouchRipple.js#L8

eps1lon commented 4 years ago

I think making the speed configurable as suggest by @arsen in https://github.com/mui-org/material-ui/issues/19664#issuecomment-586431658 is justified.

oliviertassinari commented 4 years ago

@arsen Why do you need to configure the duration?

arsen commented 4 years ago

@oliviertassinari It may sound weird but having the DURATION 550ms (plus DELAY_RIPPLE 80ms) on mobile devices feels that the app is running slow.

oliviertassinari commented 4 years ago

@arsen What would be a better value? Would it make sense to apply such value by default?

oliviertassinari commented 3 years ago

The current implementation of the ripple doesn't seem aligned anymore with how Google implements it on its product, or on the Material Design spec. Nowadays, the ripple seems to start with a wider surface and to move faster. I have added a screen recorder on the issue's description to better compare the "feeling".

aeharding commented 3 years ago

Just wanted to say it would be awesome if the ripple became a first class component like in Google's material-design. It would make the ripple much more versatile for things like large ripple surfaces (see #24310) - I closed that as a duplicate of this issue because aligning the ripple style with Material's spec would fix that. :)

mbrookes commented 3 years ago

The current implementation of the ripple doesn't seem aligned [with] the Material Design spec

I think you missed the comment* at the beginning that the live examples in the spec (such as the one you included the screen recording for) are MCW. Compare instead the list videos that @Studio384 mentioned.

*This time round at least. You previously upvoted it.

oliviertassinari commented 3 years ago

@mbrookes I have seen Google Android applications go in the same direction. I also can't find anything about the ripple on https://material.io/.

mbrookes commented 3 years ago

I also can't find anything about the ripple on https://material.io/.

Yes, like I said here: https://github.com/mui-org/material-ui/issues/19664#issuecomment-586295050

ghost commented 3 years ago

@arsen What would be a better value? Would it make sense to apply such value by default?

Is there any development plan for making the speed configurable

oliviertassinari commented 3 years ago

@codewangshuhao Now that we have dynamic styles, it's technically easier. But, how and why would you like to change the value?

Regarding solving this issue, how about we move toward making the ripple less noticeable, closer to material.io?

diff --git a/packages/material-ui/src/ButtonBase/TouchRipple.js b/packages/material-ui/src/ButtonBase/TouchRipple.js
index 3d219bb956..16d0564949 100644
--- a/packages/material-ui/src/ButtonBase/TouchRipple.js
+++ b/packages/material-ui/src/ButtonBase/TouchRipple.js
@@ -8,18 +8,22 @@ import useThemeProps from '../styles/useThemeProps';
 import Ripple from './Ripple';
 import touchRippleClasses from './touchRippleClasses';

-const DURATION = 550;
+const DURATION = 400;
 export const DELAY_RIPPLE = 80;

 const enterKeyframe = keyframes`
   0% {
-    transform: scale(0);
-    opacity: 0.1;
+    transform: scale(0.15);
+    opacity: 0.01;
+  }
+
+  33% {
+    opacity: 0.13;
   }

   100% {
     transform: scale(1);
-    opacity: 0.3;
+    opacity: 0.24;
   }
 `;

@@ -74,7 +78,7 @@ export const TouchRippleRipple = experimentalStyled(
   position: absolute;

   &.${touchRippleClasses.rippleVisible} {
-    opacity: 0.3;
+    opacity: 0.24;
     transform: scale(1);
     animation-name: ${enterKeyframe};
     animation-duration: ${DURATION}ms;

This is a change that I think can only be reviewed by applying it and trying it out.

ghost commented 3 years ago

@codewangshuhao Now that we have dynamic styles, it's technically easier. But, how and why would you like to change the value?

Regarding solving this issue, how about we move toward making the ripple less noticeable, closer to material.io?

diff --git a/packages/material-ui/src/ButtonBase/TouchRipple.js b/packages/material-ui/src/ButtonBase/TouchRipple.js
index 3d219bb956..16d0564949 100644
--- a/packages/material-ui/src/ButtonBase/TouchRipple.js
+++ b/packages/material-ui/src/ButtonBase/TouchRipple.js
@@ -8,18 +8,22 @@ import useThemeProps from '../styles/useThemeProps';
 import Ripple from './Ripple';
 import touchRippleClasses from './touchRippleClasses';

-const DURATION = 550;
+const DURATION = 400;
 export const DELAY_RIPPLE = 80;

 const enterKeyframe = keyframes`
   0% {
-    transform: scale(0);
-    opacity: 0.1;
+    transform: scale(0.15);
+    opacity: 0.01;
+  }
+
+  33% {
+    opacity: 0.13;
   }

   100% {
     transform: scale(1);
-    opacity: 0.3;
+    opacity: 0.24;
   }
 `;

@@ -74,7 +78,7 @@ export const TouchRippleRipple = experimentalStyled(
   position: absolute;

   &.${touchRippleClasses.rippleVisible} {
-    opacity: 0.3;
+    opacity: 0.24;
     transform: scale(1);
     animation-name: ${enterKeyframe};
     animation-duration: ${DURATION}ms;

This is a change that I think can only be reviewed by applying it and trying it out.

thanks for reply,For my usage scenario, I’m just confused about DURATION by the inconsistency between material-ui and material.io, and when DURATION is equal to 500ms, I feel a bit stuck when clicking the button. If material-ui can lower the DURATION value, then the configurable DURATION will not Apply to my usage scenario

oliviertassinari commented 3 years ago

@codewangshuhao Do you think that you could: 1. fork the repo 2. run yarn 3. run yarn start 4. apply the patch 5. open http://0.0.0.0:3000/components/buttons to see if my proposed patch feels better or worse?

oliviertassinari commented 3 years ago

They have updated the implementation: https://next.vuetifyjs.com/en/components/buttons/#flat.

ghost commented 3 years ago

@codewangshuhao Do you think that you could: 1. fork the repo 2. run yarn 3. run yarn start 4. apply the patch 5. open http://0.0.0.0:3000/components/buttons to see if my proposed patch feels better or worse?

I have tried this solution, and I think the stuttering feeling has improved significantly, but the click feel of the button is still not the same as that of material.io[just for myself,i will only use it on pc]

but as the ui framework,we should discuss the final realization plan with more developer,and change it more carefully.

oliviertassinari commented 3 years ago

@codewangshuhao Your description matches my experience with the diff. It feels better and it's not matching material.io, for instance, they have a much faster transition (I didn't have the time to aim for it).

ghost commented 3 years ago

Personally think that the best effect is from the latest material-components-web-components,especially for button with router.push @oliviertassinari

Immortalin commented 2 years ago

If you try Svelte's material components binding on touchscreen, the difference becomes really obvious.

https://sveltematerialui.com/

MUI's ripple comes in half a second after the thumb has touched the screen while in the official components the ripple is instantaneous. The MUI app drawer is also slower and less snappy. When scrolling through lists the MUI's ripple effect doesn't trigger half the time.

oliviertassinari commented 2 years ago

When scrolling through lists the MUI's ripple effect doesn't trigger half the time.

@Immortalin This is the whole problem, a ripple when scrolling doesn't make sense on mobile and desktop. Material UI mitigates this problem by adding a small delay before triggering the ripple, hence the delay you notice.

Immortalin commented 2 years ago

Can the delay be disabled via a prop? Because the experience is completely different on mobile. When on mobile and scrolling, the official material components feels much snappier because of the ripple. The official implementations (and also vuetify) feels almost indistinguishable from how native material on Android/iOS Flutter works. All the touch effects are there. I understand the ripple stuff doesn't make sense and isn't obvious in a desktop context but it is critical on touchscreens.

oliviertassinari commented 2 years ago

@Immortalin To be clear, my previous comment was about mobile. Do you have a recording of the problem. I personally fail to envision the exact problem that you are facing.

Immortalin commented 2 years ago

https://user-images.githubusercontent.com/7126128/153626191-6dcf9260-aa69-49de-9812-f9f600664ef5.mp4

https://user-images.githubusercontent.com/7126128/153626234-7cea810c-d2ac-43da-a556-82423754341d.mp4

https://user-images.githubusercontent.com/7126128/153626344-f4b5f85d-55dd-46c1-9fc8-4673339df5de.mp4

oliviertassinari commented 2 years ago

OK thanks, I'm resolving this thread. The ripple in the docs is disabled, so this is off topic.

@danilo-leal I think that this illustrates that there are upsides to have the whole docs use the default look and feel => less confusion among users. I'm glad we are moving in this direction 👍.

Immortalin commented 2 years ago

Regarding the constants, this might be helpful @oliviertassinari

https://github.com/material-components/material-components-web/blob/e00181e59cb1f29f4a8280f48b377a667a3e783b/packages/mdc-ripple/constants.ts#L44