Refactor Session.resendValidationLink and the ResendValidationForm to use the new API
#10556
Conversation
melvin-bot
bot
requested review from
puneetlath
and removed request for
a team
|
@shawnborton picking up from this:
Where is this red brick road message located? |
|
Here is some good reading: #9931 |
@shawnborton I see! That component is intended to be used with Pattern B, but the resend validation link flow is actually Pattern C, so this component isn't entirely reusable. What I'll do is refactor the red brick/text styles from the |
|
@shawnborton Ended up creating a
Also made sure that I didn't break the |
|
@shawnborton @bondydaa @aldo-expensify Updated the screenshots in the OP to include the new dot indicator message styles, ready for another review, thanks in advance :) |
|
@iwiznia Requesting you for review as well since I touched the |
|
I tested in Android device and the styles are looking good for the error/success message |
|
updated again, here are more screenshots of desktop |
| </Text> | ||
| </View> | ||
| {!_.isEmpty(props.account.message) && ( | ||
| <DotIndicatorMessage style={[styles.mb5]} type="success" messages={{[DateUtils.getMicroseconds()]: props.account.message}} /> |
There was a problem hiding this comment.
NAB: I don't like that key changing on every render, but considering the code in DotIndicatorMessage, this won't cause any issue at the moment. If the code changes in DotIndicatorMessage and the errors keys where used as the key prop in the map creating the <Text> this could create performance issues:
The code in DotIndicatorMessage could change like that at some point in the future because using array indexes as keys is not recommended and using the error key as the key prop would have been better.
--- but all this has low importance => NAB
(There is not benefit on using DateUtils.getMicroseconds() vs just 0 as a key because you only have one error, so sorting doesn't matter)
There was a problem hiding this comment.
I agree we should use a constant and also NAB
There was a problem hiding this comment.
👍 yeah I hear ya on the potential performance issue, I can add a comment as a "beware" here if you think it might help.
(There is not benefit on using DateUtils.getMicroseconds() vs just 0 as a key because you only have one error, so sorting doesn't matter)
and yeah I agree, we could use anything as the key in this object 0, abc, etc. I'll update that to make it simpler.
| .map(key => this.props.account.errors[key]) | ||
| .first() | ||
| .value(); | ||
| const error = formErrorTranslated || ErrorUtils.getLatestErrorMessage(this.props.account); |
There was a problem hiding this comment.
I still don't get why we only care about one error... NAB though since this existed before
| </Text> | ||
| </View> | ||
| {!_.isEmpty(props.account.message) && ( | ||
| <DotIndicatorMessage style={[styles.mb5]} type="success" messages={{[DateUtils.getMicroseconds()]: props.account.message}} /> |
There was a problem hiding this comment.
I agree we should use a constant and also NAB
|
@bondydaa cool if I assign you this PR? that way you also get contributions (I think) when it gets merged. |
contributions are about commits not who's assigned so we'll both get them for the commits that we author'd (or co-author'd) that get merged. 😉 |
|
ready for another review 🙏 |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
This PR specifically this commit might have caused this regression. |
|
I think you linked the wrong thing on Yeah I agree this was probably the cause, we should always be sure to test with 0, 1, many workspace members (and errors) to ensure the border is displayed properly. |
Oops my mistake. Updated the comment. |
| key: ONYXKEYS.ACCOUNT, | ||
| value: { | ||
| isLoading: false, | ||
| message: Localize.translateLocal('resendValidationForm.linkHasBeenResent'), |
There was a problem hiding this comment.
Hi ✋ coming from #17216, where this message is not translated when a user changes the language.
Localized text saved in Onyx is not translated when the locale is changed. So instead, we should save the key and translate it at the component level.
Details
Fixed Issues
$ https://github.com/Expensify/Expensify/issues/211841
Tests
~/Web-Expensify/_config.local.phpto whitelist this email address for dev.
6. Now run `vssh php /vagrant/Web-Expensify/script/notifyall.php` to send out the validation link emails. 7. Verify that [EmailOnDeck](https://www.emailondeck.com/) has received 2 emails with subject "Your magic sign in link for Expensify".
8. Now back on NewDot, click "Resend link" a bunch of times. Eventually, you'll get an error that should display like so:PR Review Checklist
Contributor (PR Author) Checklist
### Fixed Issuessection aboveTestssectionQA stepssectiontoggleReportand notonIconClick)src/languages/*filesSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)/** comment above it */displayNamepropertythisproperly 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)PR Reviewer Checklist
The Contributor+ will copy/paste it into a new comment and complete it after the author checklist is completed
### Fixed Issuessection aboveTestssectionQA stepssectiontoggleReportand notonIconClick).src/languages/*filesSTYLE.md) were followedAvatar, I verified the components usingAvatarhave been tested & I retested again)/** comment above it */displayNamepropertythisproperly 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)QA Steps
Same as the testing steps, except you can skip steps 2 and 6.
Screenshots
Web
Mobile Web
Desktop
iOS
Android