miljodir / md-components

Design-komponenter for Miljødirektoratet (CSS og React)

https://miljodir.github.io/md-components

MIT License

3 stars 2 forks source link

RB-2548: Only generate input id once #112

Closed turiddahl closed 7 months ago

turiddahl commented 7 months ago

Describe your changes

Changes autocompleteIdto use state so that the id is not re-generated on each render.

This code change was done because utilizing the refin a parent to retrieve the id became unnecessarily complex when the input's id rapidly changed. It is also generally not a good idea to have html elements' ids change on every render since they are made to identify the element.

Checklist before requesting a review

[X] I have performed a self-review and test of my code
[X] I have added label to the PR (major, minor or patch)
[ ] If new component: Is story for component created in stories-folder?
[ ] If new component: Is tsx-file import added to packages/react/index.tsx?
[ ] If new component: Is css-file added to packages/css/index.css?

github-actions[bot] commented 7 months ago

Please set a versioning label of either major, minor, or patch to the pull request.

aurorascharff commented 7 months ago

@kajsaeggum Har du noen inspill her? Du la til forwardRef på de andre komponentene da du trengte det, men de bruker ikke state rundt id-genereing.

kajsaeggum commented 7 months ago

@kajsaeggum Har du noen inspill her? Du la til forwardRef på de andre komponentene da du trengte det, men de bruker ikke state rundt id-genereing.

Hvis du trenger id'n til komponenten fra utsiden, ville jeg heller generert id'n på utsiden og sendt den inn i id-propertien. Altså ikke bruke ref for å få tak i id'n! Da er det opp til deg å sørge for at den id-genereringen din på utsiden ikke skjer på ny. Internt innad i denne komponenten er det ikke problematisk at id'n re-genereres slik jeg kan se det. Pleier å bruke React sin useId selv, ikke uuid sin, men kan ikke tenke meg at det er en heavy operasjon.

turiddahl commented 7 months ago

@kajsaeggum Har du noen inspill her? Du la til forwardRef på de andre komponentene da du trengte det, men de bruker ikke state rundt id-genereing.

Hvis du trenger id'n til komponenten fra utsiden, ville jeg heller generert id'n på utsiden og sendt den inn i id-propertien. Da er det opp til deg å sørge for at den id-genereringen din på utsiden ikke skjer på ny. Internt innad i denne komponenten er det ikke problematisk at id'n re-genereres slik jeg kan se det. Pleier å bruke React sin useId selv, ikke uuid sin, men kan ikke tenke meg at det er en heavy operasjon.

Ja det er sant! Jeg kan generere id-en i parent og sende inn ja 😄

Men tenker fortsatt at det er rart at den regenererer id på hver render. Tror ikke det er best practice 🤔 Virker litt mystisk at komponenten får ny id for hver bokstav man skriver inn eller hvis man click-er på den. Det er nok ikke et problem som du nevner, men er litt rart

turiddahl commented 7 months ago

Siden det ikke skaper et problem pr nå så tenker jeg at jeg lukker denne PRen. Men det kan være fint å ha i bakhodet og vurdere om vi evt vil endre på id genereringa i md komponentene