mui / material-ui

Material UI: Ready-to-use foundational React components, free forever. It includes Material UI, which implements Google's Material Design.
https://mui.com/material-ui/
MIT License
91.86k stars 31.57k forks source link

[docs][autocomplete] Fix duplicate autocomplete id #42086

Closed oliviertassinari closed 2 weeks ago

oliviertassinari commented 2 weeks ago

We can't have duplicated id on a page. I noticed this in #42085. Part of the value of solving these errors, the more we have, the harder its to quickly spot regressions

SCR-20240501-pstw

https://validator.w3.org/nu/?doc=https%3A%2F%2Fdeploy-preview-39603--material-ui.netlify.app%2Fmaterial-ui%2Freact-autocomplete%2F

A regression from #34066

After: https://deploy-preview-42086--material-ui.netlify.app/material-ui/react-autocomplete/

mui-bot commented 2 weeks ago

Netlify deploy preview

https://deploy-preview-42086--material-ui.netlify.app/

Bundle size report

No bundle size changes (Toolpad) No bundle size changes

Generated by :no_entry_sign: dangerJS against cf6704ef7fc782c458719df6a8aa24d342c9fb3b

michaldudak commented 2 weeks ago

Why do we even need the explicit id? It doesn't add anything of value to the demo.

oliviertassinari commented 2 weeks ago

@michaldudak The id was needed: https://github.com/mui/material-ui/pull/18283#discussion_r344488509 (a side though: seeing this reinforces the idea that we should comment as much as possible, such a nice payoff 4 years after).

But since useId() was introduced in React 18: https://react.dev/blog/2022/03/29/react-v18#useid, the problem got solved by React.

It's a bit borderline to remove in the docs:

SCR-20240502-ozhb

https://tools-public.mui.com/prod/pages/npmVersion?package=react-dom

but I guess, why not? I have pushed a new commit.

michaldudak commented 2 weeks ago

I understand the id is needed to wire the input to the label, but even if we use React 17 and automatically set the id only on the client, it's not really a problem. There won't be any layout shifts and I think it's safe to assume users with assistive technologies will need this in the brief moment before hydration.

oliviertassinari commented 2 weeks ago

There won't be any layout shifts and I think it's safe to assume users with assistive technologies will need this in the brief moment before hydration.

Fair enough.