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.

Sign up for GitHub

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

Jump to bottom

[material-ui][Avatar] Simplify valid children assertion #40834

Merged
merged 4 commits into from
Feb 4, 2024
Merged

[material-ui][Avatar] Simplify valid children assertion #40834

merged 4 commits into from
Feb 4, 2024
+5 −1

Conversation

oliviertassinari
Copy link
Member

@oliviertassinari oliviertassinari commented Jan 28, 2024 •
edited
Loading

@oliviertassinari oliviertassinari added performance component: avatar This is the name of the generic UI component, not the React module! enhancement This is not a bug, nor a new feature labels Jan 28, 2024
@oliviertassinari oliviertassinari mentioned this pull request Jan 28, 2024
Merged
2 tasks
@mui-bot
Copy link

mui-bot commented Jan 28, 2024 •
edited
Loading

Netlify deploy preview

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

Bundle size report

Details of bundle changes (Toolpad)
Details of bundle changes

Generated by 🚫 dangerJS against 1e6aa05

@oliviertassinari oliviertassinari force-pushed the avatar-simplify-assertion branch 2 times, most recently from d062394 to 5ab8085 Compare January 28, 2024 23:16
@danilo-leal danilo-leal changed the title [avatar] Simplify valid children assertion [material-ui][Avatar] Simplify valid children assertion Jan 29, 2024
@danilo-leal danilo-leal added the package: material-ui Specific to @mui/material label Jan 29, 2024
ZeeshanTamboli
ZeeshanTamboli reviewed Feb 2, 2024
View reviewed changes
Copy link
Member

@ZeeshanTamboli ZeeshanTamboli left a comment •
edited
Loading

Choose a reason for hiding this comment

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

@oliviertassinari IMO, sacrificing readability for such a marginal reduction in bundle size isn't justified. Even with a comment, this code takes time to understand. Instead, we can spend time optimizing bundle size in other areas while maintaining code clarity.

@oliviertassinari
Copy link
Member Author

oliviertassinari commented Feb 4, 2024 •
edited
Loading

sacrificing readability for such a marginal reduction in bundle size isn't justified.

@ZeeshanTamboli I don't see the balance this way. I don't see this is more complex, it seems simpler to me (truthy values + 0, maybe because I was already exposed to it in #34853.

The primary target is to have a great experience for end-users and developers, the experience maintainers have is secondary but should be aligned to support the former.

@oliviertassinari oliviertassinari merged commit c04ba29 into mui:master Feb 4, 2024
@oliviertassinari oliviertassinari deleted the avatar-simplify-assertion branch February 4, 2024 21:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ZeeshanTamboli ZeeshanTamboli

@mirus-ua mirus-ua

Assignees
No one assigned
Labels
component: avatar This is the name of the generic UI component, not the React module! enhancement This is not a bug, nor a new feature package: material-ui Specific to @mui/material performance
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@oliviertassinari @mui-bot @ZeeshanTamboli @mirus-ua @danilo-leal