Skip to content
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

Failed KYC message #7889

Merged
merged 7 commits into from
Mar 2, 2022
Merged

Failed KYC message #7889

merged 7 commits into from
Mar 2, 2022

Conversation

ctkochan22
Copy link

@ctkochan22 ctkochan22 commented Feb 24, 2022

@nkuoch

Details

image

Fixed Issues

$ https://github.com/Expensify/Expensify/issues/193506

Tests

Needs to be tested by whatever PR uses this

No QA Steps

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

Screenshots

Web

Mobile Web

Desktop

iOS

Android

@ctkochan22 ctkochan22 requested a review from a team as a code owner February 24, 2022 06:00
@ctkochan22
Copy link
Author

This can't be the way we add links to messages right? Seems super unscalable

{props.translate('workspace.reimburse.captureNoVBACopyBeforeEmail')}
<CopyTextToClipboard
text="receipts@expensify.com"
textStyles={[styles.textBlue]}
/>
<Text>{props.translate('workspace.reimburse.captureNoVBACopyAfterEmail')}</Text>

@MelvinBot MelvinBot requested review from danieldoglas and removed request for a team February 24, 2022 06:01
@ctkochan22 ctkochan22 self-assigned this Feb 24, 2022
@ctkochan22 ctkochan22 requested a review from nkuoch February 25, 2022 06:23
@nkuoch
Copy link
Contributor

nkuoch commented Feb 25, 2022

There are React components that transform any link from a text automatically, maybe we should use something like that? What do you think @marcaaron

@marcaaron
Copy link
Contributor

This can't be the way we add links to messages right? Seems super unscalable

Oh maybe I misunderstood, but what is the scaling issue here? Could just be a UX thing where we decided at one point it would be better to have a copy email function vs. mailto: link (but I'm just guessing and don't have the original context for that change).

There are React components that transform any link from a text automatically, maybe we should use something like that? What do you think @marcaaron

I'm not sure about this library. But I know we can render html with this component here...

https://github.com/Expensify/App/blob/main/src/components/RenderHTML.js

@ctkochan22
Copy link
Author

Oh maybe I misunderstood, but what is the scaling issue here?

I think specifically having copy for before and after emails or links is unscalable, like here:

{Localize.translateLocal('termsStep.longTermsForm.fdicInsuranceBancorp')}
{' '}
{CONST.TERMS.FDIC_PREPAID}
{' '}
{Localize.translateLocal('termsStep.longTermsForm.fdicInsuranceBancorp2')}
</Text>
<Text style={[styles.mb4, styles.textMicroSupporting]}>
{Localize.translateLocal('termsStep.noOverdraftOrCredit')}
</Text>
<Text style={[styles.mb4, styles.textMicroSupporting]}>
{Localize.translateLocal('termsStep.longTermsForm.contactExpensifyPayments')}
{' '}
{CONST.EMAIL.CONCIERGE}
{' '}
{Localize.translateLocal('termsStep.longTermsForm.contactExpensifyPayments2')}
{' '}
{CONST.NEWDOT}
.
</Text>
<Text style={[styles.mb6, styles.textMicroSupporting]}>
{Localize.translateLocal('termsStep.longTermsForm.generalInformation')}
{' '}
{CONST.TERMS.CFPB_PREPAID}
{'. '}
{Localize.translateLocal('termsStep.longTermsForm.generalInformation2')}
{' '}
{CONST.TERMS.CFPB_COMPLAINT}

So for each piece of copy that has a link or email embedded in it, we have to have two variables. For before and after?

@marcaaron
Copy link
Contributor

Ok, I think I get it. I can't quite think of a better way to not break up the copy into separate variables when there's a link besides using html (which could be an option, but not sure if we have any translated html strings yet).

One cool thing about the html engine we're using is that we can get pretty fancy with custom elements. So, say you want something like a text link to get converted to a custom component you could (in theory) pass something like:

This is before <a data-email="concierge@expensify.com" data-type="copy-email">the link</a> and now after

and then add some logic to handle the attributes in the customer renderer here:

@nkuoch
Copy link
Contributor

nkuoch commented Feb 27, 2022

@tgolen what do you think about it?
Imagine we want to display a text like: "Please go to http://help.com or contact help@expensify.com".
I don't think we'd want break the sentence and do translate(pleaseGoTo) + link(help.com) + translate(orContact) + link(help@expensify.com) - it makes the translations messy...
Having a full sentence that doesn't care about links and only care about good copy and translations seems cleaner to me?
Then we could just have a wrapper that automatically detects any link and surround them with the appropriate tags?
I found https://github.com/joshswan/react-native-autolink when quickly searching, but I'm sure there are others out there, or we could even create our own if we wanted to.

@tgolen
Copy link
Contributor

tgolen commented Feb 28, 2022

it makes the translations messy...
Seems super unscalable

I'm not sure that I agree with these yet. It doesn't seem terrible to me that the translations have to be broken up. It's definitely not the end of the world to have two translated strings wrapping an email.

It does feel like something that can be improved at some point, but I don't think we need to yet (just my personal opinion).

Then we could just have a wrapper that automatically detects any link and surround them with the appropriate tags?

My concern with this is putting more complexity into place with HTML rendering and using a third-party lib especially would put us into a worse place (since the lib would sort of be a black box). My suggestions would be (in this order):

  1. Merge this PR as-is
  2. Research how common translation libraries handle things of this nature
  3. Build the solution ourselves

The idea that I like the best so far is this one from Marc, with maybe a slightly different approach: this string has [link text="a link" url="google.com"] in the middle.

But I think we should see how common translation libraries do it first. The reason is, if we want to get an organization to provide us other translations, we should be giving them translation strings that are pretty industry-standard.

@ctkochan22 ctkochan22 changed the title [WIP] Failed KYC message Failed KYC message Mar 1, 2022
nkuoch
nkuoch previously approved these changes Mar 1, 2022
Kosuke Tseng added 2 commits March 1, 2022 15:30
@nkuoch nkuoch merged commit d6e64b3 into main Mar 2, 2022
@nkuoch nkuoch deleted the ckt_kyc_failedModal branch March 2, 2022 06:03
@OSBotify
Copy link
Contributor

OSBotify commented Mar 2, 2022

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

@OSBotify
Copy link
Contributor

🚀 Deployed to staging by @nkuoch in version: 1.1.42-0 🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

@OSBotify
Copy link
Contributor

🚀 Deployed to production by @chiragsalian in version: 1.1.42-6 🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants