-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
Fix: No error message when entering a invalid email #17254
Fix: No error message when entering a invalid email #17254
Conversation
@Beamanator 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] |
@Beamanator This is my first PR, please help to take a look at it and enable 10 workflows awaiting approval |
src/languages/en.js
Outdated
@@ -98,6 +98,7 @@ export default { | |||
characterLimit: ({limit}) => `Exceeds the maximum length of ${limit} characters`, | |||
dateInvalid: 'Please enter a valid date', | |||
invalidCharacter: 'Invalid character', | |||
invalidEmail: 'Invalid 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.
@dukenv0307 We can create a custom message similar to errorMessageInvalidPhone
inside messages
, may @neil-marcellini confirm 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.
@fedirjh just wondering, why do you prefer having the error message inside messages
instead of here? I feel like here makes more sense since this is a generic set of errors
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.
@Beamanator It's just for consistency , errorMessageInvalidPhone
is located here, so we could also move it inside common.errors
to maintain consistency
Lines 757 to 759 in 4624631
messages: { | |
errorMessageInvalidPhone: `Please enter a valid phone number without brackets or dashes. If you're outside the US please include your country code (e.g. ${CONST.EXAMPLE_PHONE_NUMBER}).`, | |
}, |
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.
@fedirjh Do you want the invalidEmail
message similar to errorMessageInvalidPhone
inside messages
or leave it as it is?
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.
@dukenv0307 I will defer that to @neil-marcellini.
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 would put it inside messages
because it's more of an informative message rather than an error message, but it's not a big deal either way.
src/languages/es.js
Outdated
@@ -97,6 +97,7 @@ export default { | |||
characterLimit: ({limit}) => `Supera el límite de ${limit} caracteres`, | |||
dateInvalid: 'Ingresa una fecha válida', | |||
invalidCharacter: 'Caracter invalida', | |||
invalidEmail: 'Email inválido', |
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.
@dukenv0307, as this is your first PR, I would like to draw your attention to a checklist item that needs to be considered.
I verified any copy / text that was added to the app is correct English and approved by marketing by adding the
Waiting for Copy
label for a copy review on the original GH to get the correct copy
Therefore, we need to request a review for the correct copy on the original GitHub issue.
Reviewer Checklist
Screenshots/Videos |
@Beamanator I think this should be assigned to @neil-marcellini. |
@neil-marcellini assigning you since it seems you were assigned on the original issue (thanks for pointing that out @fedirjh ) |
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.
Look good overall. I left a few small comments. We're also waiting for copy. I'll review again once we have the copy locked down.
src/languages/en.js
Outdated
@@ -98,6 +98,7 @@ export default { | |||
characterLimit: ({limit}) => `Exceeds the maximum length of ${limit} characters`, | |||
dateInvalid: 'Please enter a valid date', | |||
invalidCharacter: 'Invalid character', | |||
invalidEmail: 'Invalid 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.
I would put it inside messages
because it's more of an informative message rather than an error message, but it's not a big deal either way.
@Beamanator @neil-marcellini Should we hold this PR until the correct copy is confirmed |
(FYI I'm not planning to review this since I'm OOO - but I couldn't remove myself from the Reviewers list lol 🤷 ) |
18b2924
to
65ac54f
Compare
@neil-marcellini I saw Davidcardoza confirmed that the message of invalid email is "Invalid email". Actually it's set to |
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 PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
🚀 Deployed to staging by https://github.com/neil-marcellini in version: 1.3.1-0 🚀
|
🚀 Deployed to staging by https://github.com/neil-marcellini in version: 1.3.1-0 🚀
|
🚀 Deployed to production by https://github.com/mountiny in version: 1.3.1-3 🚀
|
Details
No error message when entering a invalid email for a new chat
Fixed Issues
$ #16884
PROPOSAL: #16884 (comment)
Tests
Offline tests
Same above
QA Steps
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)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.Screenshots/Videos
Web
Screencast.from.11-04-2023.15.00.12.webm
Mobile Web - Chrome
Record_2023-04-11-15-01-20.mp4
Mobile Web - Safari
Desktop
iOS
Android
16884.mp4