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

@material-ui/lab/Rating - wrong rendering for value of 3.9 and precision of 0.2 #16939

Closed
neupsh opened this issue Aug 8, 2019 · 9 comments · Fixed by #17013 or #20491
Closed

@material-ui/lab/Rating - wrong rendering for value of 3.9 and precision of 0.2 #16939

neupsh opened this issue Aug 8, 2019 · 9 comments · Fixed by #17013 or #20491
Labels
component: rating This is the name of the generic UI component, not the React module! docs Improvements or additions to the documentation ready to take Help wanted. Guidance available. There is a high chance the change will be accepted

Comments

@neupsh
Copy link

neupsh commented Aug 8, 2019

Like the title says, when you have a precision of 0.2 and value of 3.9, there are only 3 starts filled instead of 4.

The following code:

import React from 'react';
import Rating from '@material-ui/lab/Rating';

export default function HalfRating() {
  return (
    <div>
      <Rating name="half-rating-1" value={3.6} precision={0.2} />
      <Rating name="half-rating-2" value={3.9} precision={0.2} />
    </div>
  );
}

Produces:
image

First one is supposed to be less stars than second one.

Example here:
https://codesandbox.io/s/material-demo-f25jl?fontsize=14

@oliviertassinari oliviertassinari added component: rating This is the name of the generic UI component, not the React module! bug 🐛 Something doesn't work labels Aug 8, 2019
@oliviertassinari
Copy link
Member

@neupsh Thank you for the report. You are right, I have designed/tested the component with two precisions values: 1 and 0.5. I haven't looked much at the 0.x variations. But it's definitely something worth supporting. We had to solve a similar issue in the past with the slider component. I believe that we can apply the same approach:

diff --git a/packages/material-ui-lab/src/Rating/Rating.js b/packages/material-ui-lab/src/Rating/Rating.js
index 26230b975..fd17dbd86 100644
--- a/packages/material-ui-lab/src/Rating/Rating.js
+++ b/packages/material-ui-lab/src/Rating/Rating.js
@@ -15,6 +15,16 @@ function clamp(value, min, max) {
   return value;
 }

+function getDecimalPrecision(num) {
+  const decimalPart = num.toString().split('.')[1];
+  return decimalPart ? decimalPart.length : 0;
+}
+
+function roundValueToPrecision(value, precision) {
+  const nearest = Math.round(value / precision) * precision;
+  return Number(nearest.toFixed(getDecimalPrecision(precision)));
+}
+
 export const styles = theme => ({
   /* Styles applied to the root element. */
   root: {
@@ -143,7 +153,7 @@ const Rating = React.forwardRef(function Rating(props, ref) {
     focus: -1,
   });

-  let value = valueProp;
+  let value = roundValueToPrecision(valueProp, precision);
   if (hover !== -1) {
     value = hover;
   }
@@ -174,7 +184,7 @@ const Rating = React.forwardRef(function Rating(props, ref) {
       percent = (event.pageX - left - ownerWindow(rootNode).pageXOffset) / (width * max);
     }

-    let newHover = Math.ceil((max * percent) / precision) * precision;
+    let newHover = roundValueToPrecision(max * percent, precision);
     newHover = clamp(newHover, precision, max);
     setState(prev =>
       prev.hover === newHover && prev.focus === newHover
@@ -380,7 +390,7 @@ const Rating = React.forwardRef(function Rating(props, ref) {
             filled: itemValue <= value,
             hover: itemValue <= hover,
             focus: itemValue <= focus,
-            checked: itemValue === Math.round(valueProp),
+            checked: itemValue === valueProp,
           },
         );
       })}

What do you think of these changes? Do you want to submit a pull request? :)

@oliviertassinari oliviertassinari added the good first issue Great for first contributions. Enable to learn the contribution process. label Aug 9, 2019
@neupsh
Copy link
Author

neupsh commented Aug 12, 2019

Thanks @oliviertassinari , I will try to create a pull request.

oliviertassinari referenced this issue Aug 20, 2019
* Wrong rendering for value of 3.9 and precision of 0.2(Fixed)

Fixes#16939

* Update Rating.js

* [Rating] Add first test case
@neupsh
Copy link
Author

neupsh commented Aug 20, 2019

Thank you @Patil2099, I had not been able to find time to work on this.

@sureshtidbit
Copy link

I still have the issue
is any chance can you check or give me an ugly solution so I can use the rating component.
also when I give

then also this issue is there

@josephcc
Copy link

Also still having this issue as of v4.9.0

@vitorioaraujo
Copy link

same problem with me, I'm using an accuracy of 0.01.
It is for an evaluation and comparison between media evaluation, so each digit influences the user's perception.

But if the accuracy was 0.1 it would help a lot

@oliviertassinari
Copy link
Member

oliviertassinari commented Feb 29, 2020

Do you have a reproduction?

I think that we should warn against any precisions value below 0.1 as the limit to perceive a change and a point where the keyword interaction become painful.
The nominal use cases for this prop is 1 and 0.5.

If somebody wants to work on it, it would make a great new warning. I have reopen so we don't forget to implement this warning.

@oliviertassinari oliviertassinari added docs Improvements or additions to the documentation ready to take Help wanted. Guidance available. There is a high chance the change will be accepted and removed good first issue Great for first contributions. Enable to learn the contribution process. bug 🐛 Something doesn't work labels Feb 29, 2020
@AlexAndriyanenko
Copy link
Contributor

Is somebody working on it, or i can take it? And have i understood it properly, it should throw warning if precision prop is less than 0.1?

@oliviertassinari
Copy link
Member

@AlexAndriyanenko You are free to take it. Yes, I think that we should warn for below 0.1 precissons :).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: rating This is the name of the generic UI component, not the React module! docs Improvements or additions to the documentation ready to take Help wanted. Guidance available. There is a high chance the change will be accepted
Projects
None yet
6 participants