-
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
Navigate to correct report on push notification tap #8353
Conversation
Ok updated, ready for reviews. |
Bump @roryabraham or @marcaaron for a review please! |
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.
Mostly have a question or two here, but changes look good.
src/libs/Navigation/Navigation.js
Outdated
*/ | ||
function isReady() { | ||
return navigationRef.isReady(); | ||
} |
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.
noting: we have a usage of navigationRef.isReady()
in the canNavigate()
method. Looks like they're kind of doing the same thing - but one is logging if we aren't ready. Is that something we should use instead of creating a new method?
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 can use canNavigate
instead
Navigation.navigate(ROUTES.getReportRoute(reportID)); | ||
} else { | ||
// Navigation container is not yet ready, use deeplinking to open to correct report instead | ||
Navigation.setDidTapNotification(); |
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'm trying to remember why we have setDidTapNotification()
, but can't quite recall. Is it not something we need in the first part of this if/else and only when we are using deep linking? Maybe we can leave a comment if it's not too hard to figure out.
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.
IIRC the reason we have setDidTapNotification
is:
// Navigation container is not yet ready, use deeplinking to open correct report instead.
// By default, the main drawer will start in the open state when the navigation container mounts.
// So you'll see the LHN, but we instead want to link to a specific report.
// We use the `setDidTapNotification` flag here to force the main drawer to mount in the closed state.
Co-authored-by: Marc Glasser <marc.aaron.glasser@gmail.com>
I didn't have time to test this but don't want to block. Code LGTM so I'm merging. |
✋ 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 @roryabraham in version: 1.1.52-0 🚀
|
🚀 Deployed to production by @roryabraham in version: 1.1.52-0 🚀
|
Details
Tweaks the navigation for push notifications to navigate to the correct report when a push notification is tapped.
Fixed Issues
$ #6079
Tests
PR Review Checklist
Contributor (PR Author) Checklist
### Fixed Issues
section aboveTests
sectionQA steps
sectiontoggleReport
and notonIconClick
)src/languages/*
filesSTYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)/** comment above it */
displayName
propertythis
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)PR Reviewer Checklist
### Fixed Issues
section aboveTests
sectionQA steps
sectiontoggleReport
and notonIconClick
).src/languages/*
filesSTYLE.md
) were followed/** comment above it */
displayName
propertythis
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)QA Steps
Screenshots
iOS
RPReplay_Final1648752525.MP4