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

[core] Fix a few more key spread issues #42168

Merged
merged 1 commit into from
May 21, 2024

Conversation

oliviertassinari
Copy link
Member

@oliviertassinari oliviertassinari commented May 8, 2024

A quick iteration on #39833.

There are a lot more issues that I didn't fix. We will need to cherry-pick to master.

@oliviertassinari oliviertassinari added bug 🐛 Something doesn't work external dependency Blocked by external dependency, we can’t do anything about it core Infrastructure work going on behind the scenes labels May 8, 2024
@mui-bot
Copy link

mui-bot commented May 8, 2024

Netlify deploy preview

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

Bundle size report

Details of bundle changes (Toolpad)
Details of bundle changes

Generated by 🚫 dangerJS against be5dfd2

) => React.HTMLAttributes<HTMLLIElement>;
) => React.HTMLAttributes<HTMLLIElement> & { key: any };
Copy link
Member Author

Choose a reason for hiding this comment

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

To propagate in Base UI, no?

Choose a reason for hiding this comment

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

would & {key: Key} have been more appropriate here?

The introduction of this any is causing me linter problems when trying to use the same pattern in my code and pass the key explicitly in renderOption.

Comment on lines 627 to 632
// Need to clearly apply key because of https://github.com/vercel/next.js/issues/55642
const { key, ...otherProps } = props2;
return (
<li {...props2} key={props2.key}>
<li key={key} {...otherProps}>
{getOptionLabel(option)}
</li>
Copy link
Member Author

Choose a reason for hiding this comment

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

Revert #40968 because of #39833 (comment).

@oliviertassinari oliviertassinari added the needs cherry-pick The PR should be cherry-picked to master after merge label May 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work core Infrastructure work going on behind the scenes external dependency Blocked by external dependency, we can’t do anything about it needs cherry-pick The PR should be cherry-picked to master after merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants