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

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

Closed
arsen opened this issue Feb 12, 2020 · 30 comments
Closed

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

arsen opened this issue Feb 12, 2020 · 30 comments
Labels
component: ButtonBase The React component. design: material This is about Material Design, please involve a visual or UX designer in the process new feature New feature or request out of scope The problem looks valid but we won't fix it (maybe we will revisit it in the future) priority: important This change can make a difference

Comments

@arsen
Copy link

arsen commented Feb 12, 2020

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
Copy link
Member

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" 🙂)?

@mbrookes mbrookes added the status: waiting for author Issue with insufficient information label Feb 12, 2020
@Studio384
Copy link
Contributor

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.

@oliviertassinari oliviertassinari added the design: material This is about Material Design, please involve a visual or UX designer in the process label Feb 13, 2020
@mbrookes
Copy link
Member

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
Copy link
Author

arsen commented Feb 14, 2020

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 eps1lon added component: ButtonBase The React component. new feature New feature or request and removed status: waiting for author Issue with insufficient information labels Mar 1, 2020
@eps1lon
Copy link
Member

eps1lon commented Mar 1, 2020

I think making the speed configurable as suggest by @arsen in #19664 (comment) is justified.

@oliviertassinari
Copy link
Member

@arsen Why do you need to configure the duration?

@arsen
Copy link
Author

arsen commented Mar 5, 2020

@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
Copy link
Member

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

@oliviertassinari
Copy link
Member

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
Copy link

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
Copy link
Member

mbrookes commented Jan 8, 2021

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
Copy link
Member

@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/.

@oliviertassinari oliviertassinari added the priority: important This change can make a difference label Jan 8, 2021
@oliviertassinari oliviertassinari changed the title Inconsistent ripple style with the material specs [ButtonBase] Inconsistent ripple style with the material specs Jan 8, 2021
@mbrookes
Copy link
Member

mbrookes commented Jan 9, 2021

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

Yes, like I said here: #19664 (comment)

@ghost
Copy link

ghost commented Apr 15, 2021

@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
Copy link
Member

@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
Copy link

ghost commented Apr 16, 2021

@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
Copy link
Member

@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
Copy link
Member

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

@ghost
Copy link

ghost commented Apr 19, 2021

@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
Copy link
Member

oliviertassinari commented Apr 19, 2021

@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).

@oliviertassinari oliviertassinari added this to the v5 milestone Apr 20, 2021
@ghost
Copy link

ghost commented Jun 28, 2021

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

@oliviertassinari oliviertassinari removed this from the v5.1 milestone Nov 10, 2021
@Immortalin

This comment was marked as off-topic.

@oliviertassinari

This comment was marked as off-topic.

@Immortalin

This comment was marked as off-topic.

@oliviertassinari

This comment was marked as off-topic.

@Immortalin

This comment was marked as off-topic.

@oliviertassinari

This comment was marked as off-topic.

@Immortalin

This comment was marked as off-topic.

@siriwatknp
Copy link
Member

This is specific to MD2, I don't think we will have time to revisit this ever so I'm closing this.

Copy link

This issue has been closed. If you have a similar problem but not exactly the same, please open a new issue.
Now, if you have additional information related to this issue or things that could help future readers, feel free to leave a comment.

Note

@arsen How did we do? Your experience with our support team matters to us. If you have a moment, please share your thoughts in this short Support Satisfaction survey.

@siriwatknp siriwatknp added the out of scope The problem looks valid but we won't fix it (maybe we will revisit it in the future) label Dec 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: ButtonBase The React component. design: material This is about Material Design, please involve a visual or UX designer in the process new feature New feature or request out of scope The problem looks valid but we won't fix it (maybe we will revisit it in the future) priority: important This change can make a difference
Projects
None yet
Development

No branches or pull requests

8 participants