-
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
Update en.ts #44418
Update en.ts #44418
Conversation
… jamesdeanexpensify-patch-8
@carlosmiceli Please 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] |
@carlosmiceli i might've messed this up when trying to resolve conflicts. I thought I knew how to do that, but now the file changes show that I'm just trying to delete a bunch of text in some places. That's not the case. I'm mainly updating copy and not deleting full lines of copy, so that's wrong! |
Is this the problem? |
Let's see if we can figure it out / learn together ho to fix it 😄 Would this commit have the content you wanted? It's right before merging main (which is where it overwrote the work you did I think). |
hmmm...it has some of what I want. I think this might be the problem, because from what I can tell, all it's doing is deleting lines and not replacing them with anything, right? I didn't want to do that. |
Sorry, I pasted the wrong link, I meant here: https://github.com/Expensify/App/pull/44418/files Does that have everything you wanted? |
Oh, no, I thought the link was going to show you the files state of the specific commit I wanted but it showed you the most recent one. Let me do something quick and see if this is what you want... |
5ba9dbd
to
b53a8ab
Compare
@jamesdeanexpensify is this looking right now? https://github.com/Expensify/App/pull/44418/files |
This looks more like it, thank you so much! |
You got it! Do you want me to approve/merge this now? |
that would be great, thank you! |
@jamesdeanexpensify I think we still need Design's review, so I just reviewed but I'll let you take it from here :) |
Reviewer Checklist
Screenshots/VideosAndroid: NativeAndroid: mWeb ChromeiOS: NativeiOS: mWeb SafariMacOS: Chrome / SafariMacOS: Desktop |
@carlosmiceli i think because these are simple copy changes, we don't need design review, so I'll just merge! |
lastName: 'Please enter a valid last name.', | ||
noDefaultDepositAccountOrDebitCardAvailable: 'Please add a default deposit bank account or debit card.', | ||
validationAmounts: 'The validation amounts you entered are incorrect. Please double-check your bank statement and try again.', | ||
youNeedToSelectAnOption: 'Please select an option to proceed', |
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.
@jamesdeanexpensify Was it intentional to remove the period after the error messages?
I can see a rule in our codebase Use periods at the end of error messages.
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.
Sorry, I can see the PR is reverted. You can ignore my previous comment.
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.
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 am not sure how this rule is configured and whether it has an option to set based on line/sentences. Quick fix is:
You could add // eslint-disable-next-line rulesdir/use-periods-for-error-messages
before every line.
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 might hold off on doing anything for now - i'm not an engineer and i don't want to mess with anything! haha
🚀 Deployed to staging by https://github.com/carlosmiceli in version: 9.0.4-0 🚀
|
🚀 Cherry-picked to staging by https://github.com/tgolen in version: 9.0.4-5 🚀
@Expensify/applauseleads please QA this PR and check it off on the deploy checklist if it passes. |
🚀 Deployed to production by https://github.com/Julesssss in version: 9.0.5-13 🚀
|
🚀 Deployed to production by https://github.com/thienlnam in version: 9.0.6-8 🚀
|
Details
Fixed Issues
$
PROPOSAL:
Tests
Offline tests
QA Steps
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodSTYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)Design
label and/or tagged@Expensify/design
so the design team can review the changes.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.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
MacOS: Desktop