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

[Rating] Make input ids less predictable #26493

Merged
merged 6 commits into from Jun 20, 2021

Conversation

eps1lon
Copy link
Member

@eps1lon eps1lon commented May 28, 2021

Incomplete. Please review by commit.

Previous usage of useId was incompatible with the upcoming React.useOpaqueIdentifier. We can refactor the item() factory to a <Item /> component which allows us to to use React.useOpaqueIdentifier for each item.

This may break tests if you relied on stable ids for the individual radio inputs. However, you can use [value] attribute selectors instead which also means you no longer have to provide an explicit name in your tests:

-container.querySelector('input[name="rating-test-2"]')
+container.querySelector('input[value="2"]')

(This example queries for the input for the rating value "2")

Given that your container only has a single input of course. Otherwise you probably should provide an explicit name.

Preview: https://deploy-preview-26493--material-ui.netlify.app/components/rating/

@mui-pr-bot
Copy link

mui-pr-bot commented May 28, 2021

Details of bundle changes (experimental)

@material-ui/core: parsed: +0.08% , gzip: +0.09%
@material-ui/lab: parsed: +0.10% , gzip: +0.08%

Generated by 🚫 dangerJS against 3be8c95

@oliviertassinari oliviertassinari added the component: rating This is the name of the generic UI component, not the React module! label May 30, 2021
@eps1lon eps1lon added the new feature New feature or request label May 31, 2021
packages/material-ui/src/Rating/Rating.js Outdated Show resolved Hide resolved
packages/material-ui/src/Rating/Rating.js Outdated Show resolved Hide resolved
@eps1lon eps1lon force-pushed the chore/Rating/input-component branch from 3121a73 to 8d562e4 Compare June 1, 2021 07:06
@oliviertassinari
Copy link
Member

Something I have noticed that changed: a double focus ring instead of a single one:

double outlined

https://deploy-preview-26493--material-ui.netlify.app/components/rating/#rating-precision

eps1lon added a commit to eps1lon/material-ui that referenced this pull request Jun 9, 2021
@eps1lon eps1lon force-pushed the chore/Rating/input-component branch from 8d562e4 to 478e696 Compare June 9, 2021 19:09
eps1lon added a commit to eps1lon/material-ui that referenced this pull request Jun 9, 2021
@eps1lon eps1lon force-pushed the chore/Rating/input-component branch from 478e696 to bc77276 Compare June 9, 2021 19:17
@eps1lon eps1lon force-pushed the chore/Rating/input-component branch from bc77276 to 6dd6488 Compare June 9, 2021 19:28
@eps1lon
Copy link
Member Author

eps1lon commented Jun 9, 2021

Something I have noticed that changed: a double focus ring instead of a single one:

double outlined

deploy-preview-26493--material-ui.netlify.app/components/rating/#rating-precision

Greatch catch 👍 Reproduced it in a visual regression test (https://www.argos-ci.com/mui-org/material-ui/builds/4211) and fixed it in 3be8c95 (#26493)

The implementation is still a bit hard to follow but it's now clearer where the odd part happens (active state is located in different components between precise (precision % 1 > 0) and non-precise (precision % 1 === 0) ratings.

@eps1lon eps1lon marked this pull request as ready for review June 9, 2021 19:59
@eps1lon eps1lon added this to the Concurrent React milestone Jun 9, 2021
Copy link
Member

@oliviertassinari oliviertassinari left a comment

Choose a reason for hiding this comment

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

I can't notice anything else. Looks good

reactjs/rfcs#32 (comment) doesn't seem to happen (it would allow the usage of a single opaque identifier).

For context, in the initial implementation of the rating, I didn't use a component for the rating items to avoid the overhead of React components, in the order of +10% CPU per extra component (but I don't have a benchmark to prove it). With precision={0.5} we render a lot of DOM, the rating doesn't seem fast to render. But I understand that without a component, we can't use useId.

If reactjs/rfcs#32 (comment) ever happen in the future 👍 for: 1. adding a perf benchmark, 2. calling RatingItem as a function, instead of a React component and measure if it makes a difference or not.

@eps1lon
Copy link
Member Author

eps1lon commented Jun 9, 2021

reactjs/rfcs#32 (comment) doesn't seem to happen (it would allow the usage of a single opaque identifier).

This does not match the latest information I was given:

Yes, all of these things are on the list to get back to.

-- reactwg/react-18#9 (reply in thread)

calling RatingItem as a function, instead of a React component and measure if it makes a difference or not.

I don't find it helpful that we constantly have to get back to this discussion. If you want to start a discussion whether Reacts lazy evaluation model vis components is useful then go for it. But starting it whenever it suits to make an argument against a change that wasn't explicitly sanctioned by you is just not helpful. So either start an RFC on the appropriate channels to get a good understanding when we should use function calls and when compoent or stop constantly opening this unconfirmed issue.

@oliviertassinari
Copy link
Member

oliviertassinari commented Jun 9, 2021

an argument against a change

I don't think that it goes against the changes done here. I was interested in the opportunity it could yield later on, once these changes are on HEAD.

this unconfirmed issue.

AFAIK, the overhead was confirmed at the root. Here are two examples where we measured it:

  1. https://github.com/mui-org/material-ui/blob/next/benchmark/browser/README.md#output React primitives vs. React components. In this very logs, the difference is +23% ±20%.
    If I run it locally, I get: "React primitives: 37 ±1%, React components: 42 ±0%". The difference is +14% ±4%.
  2. table-raw vs. table-components. I get a difference of +22% on a single run (but not ranges of uncertainty)

Capture d’écran 2021-06-09 à 23 48 17red to
Capture d’écran 2021-06-09 à 23 48 48

My question was: Will we need to care in this specific context of the Rating? Will it ever be significant? Anyway, it's not for us to consider today, but in the future :). #17001 might be one where we can have a larger impact first.

oliviertassinari added a commit to oliviertassinari/material-ui that referenced this pull request Jun 13, 2021
Copy link
Member

@oliviertassinari oliviertassinari left a comment

Choose a reason for hiding this comment

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

To unlock the PR, as it doesn't seem to get interest from other maintainers :)

@eps1lon eps1lon merged commit 443fb31 into mui:next Jun 20, 2021
@eps1lon eps1lon deleted the chore/Rating/input-component branch June 20, 2021 12:35
@siriwatknp siriwatknp mentioned this pull request Jun 23, 2021
1 task
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! new feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants