-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
Feature : Added Promote referral program messaging to OptionsSelector component #30372
Feature : Added Promote referral program messaging to OptionsSelector component #30372
Conversation
@dubielzyk-expensify @cubuspl42 One of you needs to 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] |
Would also be great if we could get a screen recording so we can see how the interaction works together :) |
Updated code and text |
Added video as well! |
cc @puneetlath |
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!
@cubuspl42 think we can get a review of this tomorrow (friday)? |
@puneetlath Yes, I'll review it in the next hour or so. Sorry for the delay. |
src/pages/ReferralDetailsPage.js
Outdated
} | ||
const contentHeader = translate(`referralProgram.${contentType}.header`); | ||
const contentBody = translate(`referralProgram.${contentType}.body`); | ||
const generatedURL = `${CONST.REFERRAL_PROGRAM.LINK}/?thanks=${account && account.primaryLogin}`; |
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 lacks character escaping!
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.
Also, could be extracted to a separate function, at least
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.
you would like to have a separate function for generatedURL?
What do you mean by this: "This lacks character escaping!"?
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.
you would like to have a separate function for generatedURL?
I think this would be slightly more readable.
What do you mean by this: "This lacks character escaping!"?
Well, this wasn't phrased accurately. We're not talking about character escaping, but rather URL encoding.
You can't just concatenate a URL prefix with an arbitrary string, as some characters in that arbitrary string may be special characters in the URL format. +
is just an example. It's a valid character in e-mail, while in an URL it represents a space.
Character escaping is a solution to a similar problem in some languages/data formats, for example in "foo\"bar"
, the inner "
is escaped.
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.
Okay, get it.
Something like this should work:
function generateURL(email) {
const baseURL = `${CONST.REFERRAL_PROGRAM.LINK}/?thanks=`;
const encodedEmail = encodeURIComponent(email);
const generatedURL = baseURL + encodedEmail;
return generatedURL;
}
function generateURL(email) {
return `${CONST.REFERRAL_PROGRAM.LINK}/?thanks=${encodeURIComponent(email)}`;
}
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.
Yeah, that will do. The second option is fine. 👍
While I know it's only a single expression, it's quite different functionally from the surrounding code; that's why I'd extract it.
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.
Updated @cubuspl42
Can I see a mobile recording of how the keyboard slides away when you click the referral banner? Love the change to a check when you click copy code. Well done! |
Screen.Recording.2023-11-15.at.15.54.37.mov |
Sorry for my bad explanation. Can you add a recording of where you click the "Refer a friend, get $250" banner? Basically what you have in the video, just tap the banner as well so I can see how the keyboard hides. Then if you can click back from the screen you go to so we can see how that works as well. |
Also, can you add a period to the end "Refer a friend, get $250." to see how it looks? Thank you! |
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 great. Just a few small comments.
@dubielzyk-expensify Here is a video, sorry for a bid delay in response Screen.Recording.2023-11-16.at.20.27.49.mov |
@puneetlath Updated all your comments |
Watching this video, I wonder if it's worth a discussion in Slack @dubielzyk-expensify @puneetlath on how much space the "Refer a friend, get $250" text takes up when you click into the search field. Any strong opinions there? |
I think it looks good. @dubielzyk-expensify feel free to merge if you are good with the video. @jamesdeanexpensify I say we try it and we can update the behavior if we think it's a problem after experiencing it for a bit. |
Sounds good to me! |
I'm good with the video (looks great), but I'll let someone else merge it 😂 |
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
buttonText1: 'Request money, ', | ||
buttonText2: `get $${CONST.REFERRAL_PROGRAM.REVENUE}.`, | ||
header: `Request money, get $${CONST.REFERRAL_PROGRAM.REVENUE}`, | ||
body: `Request money from a new Expensify account. Get $${CONST.REFERRAL_PROGRAM.REVENUE} once they start an annual subscription with two or more active members and make the first two payments toward their Expensify bill.`, |
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.
body: `Request money from a new Expensify account. Get $${CONST.REFERRAL_PROGRAM.REVENUE} once they start an annual subscription with two or more active members and make the first two payments toward their Expensify bill.`, | |
body: `Request money from a new Expensify account. Get $${CONST.REFERRAL_PROGRAM.REVENUE} once they start an annual subscription with two or more active members and make the first two payments toward their Expensify bill.`, |
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.
Anyone can raise quick fix for this typo?
🚀 Deployed to production by https://github.com/luacmartins in version: 1.4.1-13 🚀
|
Just FYI that this PR had this issue. We didn't add a fallback route to
|
@@ -180,6 +197,10 @@ class BaseOptionsSelector extends Component { | |||
this.props.onChangeText(value); | |||
} | |||
|
|||
handleReferralModal() { |
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.
Hey there! I'm refactoring this whole component and curious about its purpose. It looks like we never call it.
@@ -203,6 +203,8 @@ class SearchPage extends Component { | |||
textInputAlert={ | |||
this.props.network.isOffline ? `${this.props.translate('common.youAppearToBeOffline')} ${this.props.translate('search.resultsAreLimited')}` : '' | |||
} | |||
shouldShowReferralCTA |
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.
When this was added ScreenWrapper
should have passed the prop enableMaxHeight
. Without that the banner stays behind the on screen keyboard on mWeb causing the issue #35182
> | ||
<HeaderWithBackButton | ||
title={translate('common.referral')} | ||
onBackButtonPress={() => Navigation.goBack()} |
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.
Coming from #31866:
Deep link case should have been considered on back button press. Not simply Navigation.goBack()
Details
This PR is to add refferal program messaging to OptionsSelector component
Fixed Issues
$ #29881
Tests
Click on the magnifying glass (search button).
Click on the 'Referral' CTA button.
Verify that no errors appear in the JS console
Offline tests
Same as tests
QA Steps
Same as tests
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.Web
Video:Screen.Recording.2023-11-02.at.15.06.12_compressed.mp4
Request:
Start chat:
Send money:
Click the magnifying glass: