-
Notifications
You must be signed in to change notification settings - Fork 826
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
[EuAvatar] Avoid mutating the style prop #6251
[EuAvatar] Avoid mutating the style prop #6251
Conversation
src/components/avatar/avatar.tsx
Outdated
// copying style into a new object for `avatarStyle` has two purposes: | ||
// 1. `avatarStyle` is mutated later and this prevents that mutation from affecting the incoming prop | ||
// 2. if `style` is undefined, this "defaults" an empty object | ||
const avatarStyle: CSSProperties = { ...style }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not a strongly held change request, but I don't think this level of comment detail is necessary. The idea of not mutating objects not defined by ourselves is a fairly standard one in JS at this point and in particular I think 2. doesn't add anything to the context of this change. Nor do I think it likely that someone will come in here and change it back to a mutated obj for any reason that would warrant adding this comment.
Also worth considering: instead of setting props in avatarStyle.x
directly, we can assign avatarStyle to a let
and have the property setting be spread operators later, e.g.
let avatarStyle = style;
// later
avatarStyle = { ...avatarStyle, color: textColor, backgroundColor: assignedColor };
// later
avatarStyle = { ...avatarStyle, backgroundImage: etc };
This is a pattern we use in several other components/places: 1, 2, 3
Its primary advantage (other than not potentially unnecessarily cloning style
if no overrides exist) is not having to set the CSSProperties
type definition, as TS automatically infers the overrides.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@thomheymann has requested this fix for 8.5 so gonna go ahead and approve as my comments just food for thought / not blocking!
Applied your recommended changes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! 🎉
Preview documentation changes for this PR: https://eui.elastic.co/pr_6251/ |
Amazing, thanks so much for fixing this so quickly! |
Summary
Closes #6249 by cloning the incoming
style
prop, if any, instead of keeping a direct reference to the object.Checklist
- [ ] Checked in both light and dark modes- [ ] Checked in mobile- [ ] Checked in Chrome, Safari, Edge, and Firefox- [ ] Props have proper autodocs and playground toggles- [ ] Added documentation- [ ] Checked Code Sandbox works for any docs examples- [ ] Added or updated jest and cypress tests- [ ] Checked for accessibility including keyboard-only and screenreader modes- [ ] Updated the Figma library counterpart