-
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
Refactor/16140 migrate deeplinkwrapper to function #24090
Refactor/16140 migrate deeplinkwrapper to function #24090
Conversation
@robertjchen Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
@@ -12,9 +12,13 @@ const propTypes = { | |||
children: PropTypes.node.isRequired, | |||
}; | |||
|
|||
class DeeplinkWrapper extends PureComponent { |
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.
We can use memo
equivalent to PureComponent
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.
we don't need memo here
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.
I thought no need at first. But this actually includes entire app views as children.
So still you persist no performance optimization here?
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.
I am also fine no memo
usage but let's just guarantee that this won't cause any performance regressions.
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.
No, it shouldn't. I've checked both implementations but I noticed no difference. And memo uses shallow comparison so it won't work for props.children and will always cause a rerender.
Can you please link issues correctly not to break automation?
|
Author checklist failing. FYI: new checklist item is added recently
|
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.
Code looks good.
@robertjchen I see you started checklist. Will you go ahead or leave it to me?
@situchan Please proceed with the checklist, thank you! |
Reviewer Checklist
Screenshots/VideosWebScreen.Recording.2023-08-07.at.10.57.19.PM.movMobile Web - ChromeMobile Web - SafariDesktopiOSAndroid |
@robertjchen all yours |
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.
commenting to re-trigger actions
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.
not sure why checklist failing
Gotcha. @robertjchen can you please remove your checklist? It seems like yours is first detected as Reviewer Checklist. |
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.
Thanks for the tests, this looks good to me as well
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
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.
Woops I think this was supposed to be assigned to me as I was managing the issue. Thanks for the review though @robertjchen 🙇
🚀 Deployed to staging by https://github.com/robertjchen in version: 1.3.52-0 🚀
|
🚀 Deployed to staging by https://github.com/robertjchen in version: 1.3.52-0 🚀
|
🚀 Deployed to production by https://github.com/puneetlath in version: 1.3.52-5 🚀
|
Details
Fixed Issues
$ #16140
$ #16139
PROPOSAL: N/A
Tests
Offline tests
N/A
QA Steps
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)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
(commenting out the first condition in the useEffect)
Screen.Recording.2023-08-03.at.12.25.53.mp4
Mobile Web - Chrome
N/A
Mobile Web - Safari
N/A
Desktop
N/A
iOS
N/A
Android
N/A