[docs] Rating value is nullable in onChange

[sakura90]

• [o] I have followed (at least) the PR section of the contributing guide.

The previous PR I created somehow got closed immediately after I reverted my previous change. Please look at this PR instead.

@eps1lon you told me to look at material-ui/packages/material-ui/src/Rating/Rating.d.ts Line 82, but I think you meant I might take a look of a different line of Rating.d.ts. I couldn't understand when I looked an line 82.

Put in null in onChange in Rating, and also put in handling NullLiteral in auto generation of the descriptions of a function and its parameters.

Closes #26497

[mui-pr-bot]

No bundle size changes (experimental)

Generated by 🚫 dangerJS against 04e696c

[eps1lon]

Thanks! Should onChangeActive get the same treatment?

[sakura90]

onChangeActive shouldn't get the same treatment since value type is number in onChangeActive.

https://github.com/mui-org/material-ui/blob/next/packages/material-ui/src/Rating/Rating.d.ts#L89-L91

Not very sure the actual reason why he | is escaped. I had a reason that | is displayed without a rogue \ in the current material-ui site for Autocomplete API, and because he | is displayed with a rogue \ in Autocomplete API in the current deployed doc, the Rating API fix about null can just follow the current code of Autocomplete API. When the Rating API code follows the Autocomplete API code, I guess | is displayed without a rogue \ in the current material-ui site also for Rating API.

| is displayed without a rogue \ in the current material-ui site as T | T[] for onChange of Autocomplete API:
https://material-ui.com/api/autocomplete/

Rogue \'s are revealed as number \| null and T \| T[] in onChange of both Rating API and Autocomplete API in the deployed docs:
https://deploy-preview-26542--material-ui.netlify.app/api/rating/
https://deploy-preview-26542--material-ui.netlify.app/api/autocomplete/

[oliviertassinari]

onChangeActive shouldn't get the same treatment since value type is number in onChangeActive.

@sakura90 I think that the point that Sebastian is making is the the current prop description and type definition seems out of sync with the implementation. Isn't this callback called 1. when using the keyboard navigation and 2. with a null value is landing in the reset option?

[sakura90]

@oliviertassinari onChangeActive and onChange are spelled similarly, but they are just 2 different event listeners.

I think about 3 input scenarios - mouse, touch (from Android/iPhone), and keyboard - about these 2 event listeners.

For mouse, right now, onChange is called 1. when using the mouse selection and 2. with a null value is landing in the reset option, but onChangeActive is called 1. when using the mouse navigation and 2. with a -1 value when the position of the mouse is outside Rating component.

For touch, right now, onChange is called 1. when using the touch selection and 2. with a null value is landing in the reset option
From the prop description, onChangeActive is more for hovering, so I guess it is better not to consider it in touch, but FYI, onChangeActive is called when Rating component is in focus/blur from my understanding.

For keyboard, I don't spend too much time on both onChangeActive and onChange as it is a low frequent scenario.

Back to your question, I think it is fair to let onChangeActive be what it is. onChangeActive is mainly used for the mouse scenario. It feels unusual to see a -1 value when the position of the mouse is outside Rating component while an event listener with a similar name onChange returns a null in a similar situation. Having a new type such as OUT_OF_RANGE instead of -1 is going to be more descriptive, but because most users of onChangeActive are developers, I think developers can advance their work with -1 and onChangeActive can be left as what it is.

[sakura90]

Alright, I'll fix it in a follow-up.

@eps1lon Subtle, but nice to fix when there is time to do.

[sakura90]

@eps1lon If you don't have further comments about onChangeActive and other things, what about merging the PR?

[oliviertassinari]

@sakura90 Thanks for the exploration. I had a look too. It seems that onChangeActive shouldn't be fired on blur/focus. It's off topic but I'm leaving it here in case you want to work on a follow-up. What do you think about the below change?

diff --git a/packages/material-ui/src/Rating/Rating.js b/packages/material-ui/src/Rating/Rating.js
index 55a52464a2..744fea2e97 100644
--- a/packages/material-ui/src/Rating/Rating.js
+++ b/packages/material-ui/src/Rating/Rating.js
@@ -357,10 +357,6 @@ const Rating = React.forwardRef(function Rating(inProps, ref) {
       hover: prev.hover,
       focus: newFocus,
     }));
-
-    if (onChangeActive && focus !== newFocus) {
-      onChangeActive(event, newFocus);
-    }
   };

   const handleBlur = (event) => {
@@ -378,10 +374,6 @@ const Rating = React.forwardRef(function Rating(inProps, ref) {
       hover: prev.hover,
       focus: newFocus,
     }));
-
-    if (onChangeActive && focus !== newFocus) {
-      onChangeActive(event, newFocus);
-    }
   };

   const [emptyValueFocused, setEmptyValueFocused] = React.useState(false);

[sakura90]

The change seems to be nice. But if I am going to make the change, I am going to manually test the change before PR.

I think if it is clearer about things about onChange and onChangeActive, it is going to be better. onChange can listen to changes of mouse and touch while onChangeActive can listen to hover of only mouse. If it is clearer that ontouchmove, which is the version of hover for touch, is not supported, developers do not have to try each functionality before using it and have an easier time using the functionality.

I am going to create a new issue, assign it to myself, and work on the issue.

[eps1lon]

@sakura90 Thanks!