Feature : Added Promote referral program messaging to OptionsSelector component #30372
Conversation
melvin-bot
bot
requested review from
cubuspl42 and
dubielzyk-expensify
and removed request for
a team
|
@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] |
|
Shaping up nicely! A few things:
|
|
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 |
|
@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.
This lacks character escaping!
There was a problem hiding this comment.
Also, could be extracted to a separate function, at least
There was a problem hiding this comment.
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.
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.
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.
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.
|
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! |
|
@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.
| 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.
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.
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.
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.
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 Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)myBool && <MyComponent />.src/languages/*files and using the translation methodWaiting for Copylabel for a copy review on the original GH to get the correct copy.STYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)/** comment above it */thisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor)thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG))Avataris modified, I verified thatAvataris working as expected in all cases)ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Web
Video:Screen.Recording.2023-11-02.at.15.06.12_compressed.mp4
Request:
Start chat:
Send money:
Click the magnifying glass:
