-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
fix: sync avatarRows and props.icons using useMemo #24318
Conversation
…ows and props.icons
Reviewer Checklist
Screenshots/VideosWebCleanShot.2023-08-09.at.18.59.17.mp4Mobile Web - ChromeCleanShot.2023-08-09.at.19.16.34.mp4Mobile Web - SafariCleanShot.2023-08-09.at.19.05.54.mp4DesktopCleanShot.2023-08-09.at.19.03.02.mp4iOSCleanShot.2023-08-09.at.19.07.30.mp4AndroidCleanShot.2023-08-09.at.19.30.32.mp4 |
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.
Looks good and tests well
let avatarContainerStyles = getContainerStyles(props.size, props.isInReportAction); | ||
const singleAvatarStyle = props.size === CONST.AVATAR_SIZE.SMALL ? styles.singleAvatarSmall : styles.singleAvatar; | ||
const secondAvatarStyles = [props.size === CONST.AVATAR_SIZE.SMALL ? styles.secondAvatarSmall : styles.secondAvatar, ...props.secondAvatarStyle]; | ||
const tooltipTexts = props.shouldShowTooltip ? _.pluck(props.icons, 'name') : ['']; | ||
|
||
const calculateAvatarRows = () => { | ||
// If we're not displaying avatars in rows or the number of icons is less than or equal to the max avatars in a row, return a single row |
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.
@s-alves10 Can we keep this comment?
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.
Restored
Please take a look again |
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.
Looks good to me.
🎀 👀 🎀 C+ reviewed
Will you take a look? |
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.
Good catch. 👍
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
🚀 Deployed to staging by https://github.com/techievivek in version: 1.3.53-0 🚀
|
🚀 Deployed to production by https://github.com/yuwenmemon in version: 1.3.53-2 🚀
|
Details
Use
useMemo
instead ofuseState
anduseEffect
to synchronizeavatarRows
andprops.icons
Fixed Issues
$ #24062
PROPOSAL: #24062 (comment)
Tests
Profile
avatar to go to settings pageOffline tests
This is an issue that happens when switching to online mode. I don't think we need offline tests
QA Steps
Profile
avatar to go to settings pagePR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodWaiting for Copy
label for a copy review on the original GH to get the correct copy.STYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)/** comment above it */
this
properly so there are no scoping issues (i.e. foronClick={this.submit}
the methodthis.submit
should be bound tothis
in the constructor)this
are necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);
ifthis.submit
is never passed to a component event handler likeonClick
)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Web
24062_mac_chrome.mp4
Mobile Web - Chrome
24062_android_chrome.mp4
Mobile Web - Safari
24062_ios_safari.mp4
Desktop
24062_mac_desktop.mp4
iOS
24062_ios_native.mp4
Android
24062_android_native.mp4