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

Update copy to refer to new URL #4091

Merged
merged 13 commits into from
Aug 2, 2021
Merged

Update copy to refer to new URL #4091

merged 13 commits into from
Aug 2, 2021

Conversation

tgolen
Copy link
Contributor

@tgolen tgolen commented Jul 15, 2021

Details

Fixed Issues

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

Tests

  1. Sign into the app
  2. Go to settings
  3. Go to reset password
  4. Verify the copy mentions "new.expensify.com"
  5. Check the title of the apps (will be different in each platform) and see that they contain "new.expensify.com" now

QA Steps

Same as tests

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

Screenshots

Web

image

image

Mobile Web

image

Desktop

image
image

iOS

image

Android

image

@tgolen tgolen requested a review from a team July 15, 2021 21:32
@tgolen tgolen self-assigned this Jul 15, 2021
@MelvinBot MelvinBot requested review from MonilBhavsar and removed request for a team July 15, 2021 21:32
@MonilBhavsar
Copy link
Contributor

There are usages of Expensify.cash in Expensify.cash/config/electron.config.js and Expensify.cash/src/libs/actions/IOU.js L163.
Not sure if we need to update it.

@tgolen
Copy link
Contributor Author

tgolen commented Jul 16, 2021 via email

@tgolen tgolen requested a review from a team as a code owner July 16, 2021 20:01
@MelvinBot MelvinBot requested review from joelbettner and removed request for a team July 16, 2021 20:01
@tgolen
Copy link
Contributor Author

tgolen commented Jul 16, 2021

Hey @kevinksullivan here are the copy changes I had. There is some confusion and inconsistency about the Spanish translations (see this slack convo) which we need to try and resolve.

@joelbettner
Copy link
Contributor

I also believe there was some conversation about the new Expensify rather than new Expensify. Not sure how that got resolved.

theCode: 'el código',
openJobs: 'trabajos disponibles',
},
termsOfUse: {
phrase1: 'Al usar Expensify.cash, estás aceptando los',
phrase1: 'Al usar Nuevo Expensify, estás aceptando los',
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The english version of this leaves out the name of the app. I think we should do the same here.

English:

By logging in, you agree to the

@iwiznia @marklouisdeshaun what is the proper translation into Spanish?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By logging in, you agree to the

Al iniciar sesión, estás accediendo a los

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this read correctly, then?

image

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope, seems it's missing a line? Is this the up to date copy?

    termsOfUse: {
        phrase1: 'By logging in, you agree to the',
        phrase2: 'terms of service',
        phrase3: 'and',
        phrase4: 'privacy policy',
        phrase5: 'Money transmission is provided by Expensify Payments LLC (NMLS',
        phrase6: 'ID:2017010) pursuant to its',
        phrase7: 'licenses',
    },

If so:

    termsOfUse: {
        phrase1: 'Al usar Expensify.cash, estás aceptando los',
        phrase2: 'términos de servicio',
        phrase3: 'y',
        phrase4: 'política de privacidad',
        phrase5: 'El envío de dinero es brindado por Expensify Payments LLC (NMLS',
        phrase6: 'ID:2017010) de conformidad con sus',
        phrase7: 'licencias',
    },

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, so sorry... I was looking at the welcome text 🤦

OK, I think I've got it right if you want to check the changes in this PR.

@tgolen
Copy link
Contributor Author

tgolen commented Jul 19, 2021

I updated this again after a couple of Slack discussions. Adding a few of our Spanish experts to the review.

@tgolen
Copy link
Contributor Author

tgolen commented Jul 27, 2021

bump for review please (there are a few of you assigned)

@iwiznia iwiznia removed their request for review July 27, 2021 17:12
Copy link
Contributor

@iwiznia iwiznia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just reviewing the spanish translations

src/languages/es.js Outdated Show resolved Hide resolved
src/languages/es.js Outdated Show resolved Hide resolved
tgolen and others added 2 commits July 27, 2021 14:07
Co-authored-by: Ionatan Wiznia <ionatan@expensify.com>
Co-authored-by: Ionatan Wiznia <ionatan@expensify.com>
@tgolen
Copy link
Contributor Author

tgolen commented Jul 27, 2021

Good catches, @iwiznia. I've applied those.

@MonilBhavsar
Copy link
Contributor

This looks good except travis failing. I also found another issue trying to do the same - #4213. @tgolen, can we include that in this PR or tell them we have renamed strings and some notifications in this PR.

@@ -98,7 +98,7 @@ export default {
hello: 'Hello',
phoneCountryCode: '1',
welcomeText: {
phrase1: 'Welcome to the New Expensify! Enter your phone number or email to continue.',
phrase1: 'Welcome to New Expensify! Enter your phone number or email to continue.',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Based on David's Vatican reference, my understanding is the home page saying "the New Expensify" makes sense here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool, updated!

expensifyDotCash: 'Expensify.cash',
expensifyIsOpenSource: 'The New Expensify is open source',
expensifyDotCash: 'New Expensify',
expensifyIsOpenSource: 'New Expensify is open source',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar to my comment above, if we decide to use "the" at all, this seems like the other sensible place to do so.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we want to use "the" here because this ends up as the header of the page.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, hold on, I was looking at the line above it for expensifyDotCash.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The phrase for expensifyIsOpenSource is not referenced anywhere, so I will just remove it. I've updated this branch.

Copy link
Contributor

@kevinksullivan kevinksullivan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added two comments about the insertion of "the" based on my understanding of this conversation. Ultimately these are nitpicks, so feel free to carry on without making the tweaks.

@tgolen
Copy link
Contributor Author

tgolen commented Jul 30, 2021

OK, fixed conflicts again, can someone please merge it? It's been approved several times now 😁

@iwiznia iwiznia dismissed kevinksullivan’s stale review August 2, 2021 11:57

Changes were addressed

@iwiznia iwiznia merged commit a57f9eb into main Aug 2, 2021
@iwiznia iwiznia deleted the tgolen-password-copy branch August 2, 2021 11:57
@OSBotify
Copy link
Contributor

OSBotify commented Aug 2, 2021

✋ 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

OSBotify commented Aug 6, 2021

🚀 Deployed to staging in version: 1.0.82-8🚀

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

@OSBotify
Copy link
Contributor

🚀 Deployed to production by @francoisl in version: 1.0.83-1 🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 failure ❌
🍎 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.

6 participants