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.

Sign up for GitHub

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

Jump to bottom

implement character limit checking for custom pronouns #6899

Merged
merged 19 commits into from
Jan 10, 2022
Merged

implement character limit checking for custom pronouns #6899

merged 19 commits into from
Jan 10, 2022

Conversation

anthony-hull
Copy link
Contributor

@anthony-hull anthony-hull commented Dec 23, 2021 •
edited
Loading

Details

Fixed Issues

$ #6492

Tests

QA Steps

  1. navigate to profile screen
  2. Select chose self select pronoun from drop down
  3. Enter more that 50 characters
  4. Click save
  5. Check for input error state and "Exceeds the max length of 50 characters" error
  6. Reduce character to below 50
  7. Click save
  8. See profile saved toast
  9. click away from the profile
  10. Click back onto the profile and see the changes have been persisted
  11. Increase pronoun to 51 characters and select a pre-set in the drop down.
  12. Click save
  13. see profile saved toast

Retry the above steps for First and Last name. First and Last name will give an API error toast as well as the contextual input error state. This is not part of the change but there is some inconsistency there.

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

Screenshots

Web

image
image

Mobile Web

image

Desktop

image

iOS

image

Android

image

@anthony-hull
implement character limit checking for custom pronouns
e185731
@anthony-hull
implement character limit checking for custom pronouns
149c6d0
@anthony-hull
remove unnecessary .join()
19677ad
@anthony-hull
add spanish translation
a25cf1c 
tweak lang variable implementation to be consistent
restore [type]NameLength translations for API errors
make getCharacterLimitErrors use of personalDetails.error.characterLimit variable driven
@anthony-hull
needed the join
762aee8
@anthony-hull anthony-hull marked this pull request as ready for review December 27, 2021 14:26
@anthony-hull anthony-hull requested a review from a team as a code owner December 27, 2021 14:26
@MelvinBot MelvinBot removed the request for review from a team December 27, 2021 14:26
parasharrajat
parasharrajat suggested changes Dec 27, 2021
View reviewed changes
@anthony-hull
move errors function to validation utils
2f5ec72 
refactor error function to use destructured variables
parasharrajat
parasharrajat suggested changes Dec 30, 2021
View reviewed changes
@anthony-hull
Copy link
Contributor Author

I'll get to this tomorrow

@bfitzexpensify bfitzexpensify mentioned this pull request Jan 3, 2022
Closed
@parasharrajat
Copy link
Member

@anthony-hull Could you please fix the lint errors?

@anthony-hull
Copy link
Contributor Author

anthony-hull commented Jan 4, 2022 •
edited
Loading

the lint error is for a file I didn't change, should I be fixing lint errors for files not related to the PR?

@parasharrajat
Copy link
Member

Please merge main...

@anthony-hull
Copy link
Contributor Author

Looks to me like a use of ExpensifyText in ReportItemActionMessage.js hasn't been renamed to Text.
Should it be another PR to fix?

@parasharrajat
Copy link
Member

Yeah, Please wait. I will create a new PR to fix this.

@anthony-hull
Copy link
Contributor Author

anthony-hull commented Jan 4, 2022 •
edited
Loading

#7006
EDIT:
looks like there is one, just hasn't merged

@parasharrajat
Copy link
Member

Great. No issues. Let's wait for it to merge.

@Julesssss
Copy link
Contributor

The fix PR should be merged very soon.

@Julesssss
Copy link
Contributor

@anthony-hull that PR is merged now.

@anthony-hull
Copy link
Contributor Author

I'll re-request a review when this is ready

@anthony-hull
rename props FullNameInputRow
d0c1d6d 
rename variable in getMaxCharacterError to improve self description
@anthony-hull
rename props passed to FullNameInputRow in ProfilePage
5d2bd6d
parasharrajat
parasharrajat suggested changes Jan 5, 2022
View reviewed changes
Copy link
Member

@parasharrajat parasharrajat left a comment

Choose a reason for hiding this comment

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

So far so good. Only one change. Thanks for your patience.

parasharrajat
parasharrajat suggested changes Jan 6, 2022
View reviewed changes
Copy link
Member

@parasharrajat parasharrajat left a comment

Choose a reason for hiding this comment

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

I missed this sorry. But looks good.

parasharrajat
parasharrajat previously approved these changes Jan 6, 2022
View reviewed changes
Copy link
Member

@parasharrajat parasharrajat left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for the changes.

cc: @Julesssss

🎀 👀 🎀 C+ reviewed

@anthony-hull anthony-hull requested a review from Julesssss January 6, 2022 15:51
@Julesssss
Copy link
Contributor

I forgot to merge this 🙃

@Julesssss Julesssss merged commit b824181 into Expensify:main Jan 10, 2022
@OSBotify
Copy link
Contributor

✋ 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 OSBotify mentioned this pull request Jan 10, 2022
Closed
24 tasks
@OSBotify
Copy link
Contributor

🚀 Deployed to staging by @Julesssss in version: 1.1.26-2 🚀

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

@OSBotify
Copy link
Contributor

🚀 Deployed to staging by @Julesssss in version: 1.1.27-0 🚀

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

@mvtglobally
Copy link

@anthony-hull We are getting errors under each fields during QA.
Do you have a screenshot on how the "API error toast as well as the contextual input error state" looks like? Team was not able to get any popup message

@Julesssss
Copy link
Contributor

Hi @mvtglobally. I believe this instruction could be out of date now. Seeing the same message (Exceeds the max length of 50 characters) under the first name and last name fields is a pass in my opinion.

First and Last name will give an API error toast as well as the contextual input error state. This is not part of the change but there is some inconsistency there.

@anthony-hull
Copy link
Contributor Author

anthony-hull commented Jan 11, 2022 •
edited
Loading

I believe this instruction could be out of date now.

I agree. That instruction shouldn't be there anymore.

API error toast

Sorry I should have removed that. The form won't submit if the characters are too many. This means the server error popup should never be fired as the request shouldn't ever be made to the server.

the contextual input error state

The error below is what I was referring to when I wrote 'contextual'.

@OSBotify
Copy link
Contributor

🚀 Deployed to production by @roryabraham in version: 1.1.27-1 🚀

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

@anthony-hull anthony-hull deleted the anthony--pronoun-max-character branch January 20, 2022 14:26
@luacmartins luacmartins mentioned this pull request Jun 24, 2022
Merged
93 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Julesssss Julesssss Julesssss approved these changes

@parasharrajat parasharrajat parasharrajat left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@anthony-hull @parasharrajat @Julesssss @OSBotify @mvtglobally