-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Make sure we call OpenProfile action/API call even if we're accessing the page directly #21834
Conversation
@narefyev91 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] |
@yuwenmemon can you please update description in your comment - video for web is not loading, thanks! |
@@ -84,6 +85,10 @@ function ProfilePage(props) { | |||
}, | |||
]; | |||
|
|||
useEffect(() => { | |||
App.openProfile(props.currentUserPersonalDetails); |
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.
Why do we want to call openProfile any time props.currentUserPersonalDetails
changes? 🤔 I assumed we'd only want to call the function the first time we open the profile page - similar to the old componentDidMount
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 had an issue where myPersonalDetails
was not initialized yet when calling openProfile
so I thought maybe it would be better to just pass it in 🤔
But I guess I can return early in openProfile
if myPersonalDetails
are not there yet? What do you think?
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.
Yes you can add earlier return - that if you do not have myPersonalDetails you should not openProfile
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.
Right, but then we have a problem where we call openProfile before our personal details are loaded ...and we never call it 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.
Sorry for the delay, was ooo friday :D
Oof so I guess if we do the early return (if props. currentUserPersonalDetails
doesn't exist, return) then we'll have to keep props.currentUserPersonalDetails
as a dependency, right? I guess I'm ok with that, the current user probably won't be updating their personal details on a different device while they have this page open
@narefyev91 fixed! |
@narefyev91 would you please review today if you have the time? 🙏 |
Sure on it now |
Thank you sir 🙏 |
Reviewer Checklist
Screenshots/VideosWebweb.movMobile Web - Chromeandroid-web.movMobile Web - Safariios-web.movDesktopN/A iOSN/A AndroidN/A |
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! cc @Beamanator
@danieldoglas 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] |
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.
Aight well I think we're good here 👍 Though we might want to return early in the ProfilePage's useEffect if props. currentUserPersonalDetails
is empty, just so we don't unnecessarily call the OpenProfile API command
✋ 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/danieldoglas in version: 1.3.36-0 🚀
|
@yuwenmemon We are not sure if we can test at our end. Not seeing expected result with paid subscription. bandicam.2023-07-04.16-41-06-892.mp4 |
🚀 Deployed to production by https://github.com/Julesssss in version: 1.3.36-5 🚀
|
cc @Beamanator for your review
Details
We're not calling the
openProfile
action when we access the profile page directly. So instead, utilizeuseEffect
to call this when the page initializes.Fixed Issues
$ #21830
Tests
Offline tests
QA Steps
PR 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
Kapture.2023-06-29.at.10.26.49.mp4
Mobile Web - Chrome
Kapture.2023-06-28.at.11.29.10.mp4
Mobile Web - Safari
Kapture.2023-06-28.at.11.25.50.mp4