Skip to content

remove character counter#8145

Merged
mountiny merged 4 commits intoExpensify:mainfrom
Tushu17:remove-max-length
Mar 16, 2022
Merged

remove character counter#8145
mountiny merged 4 commits intoExpensify:mainfrom
Tushu17:remove-max-length

Conversation

@Tushu17
Copy link
Contributor

@Tushu17 Tushu17 commented Mar 14, 2022 •
edited
Loading

Details

Fixed Issues

Removed character limit counter

$ #8044

Tests

  1. Go to workspace > Connect bank account > Connect manually
  2. Go to company information page
  3. Check the fields with max character limit (ex:- SSN, zip code, phone number field) don't have any kind of character counter below when you type something.
  • Verify that no errors appear in the JS console

PR Review Checklist

Contributor (PR Author) Checklist

  • I verified the PR has a small number of commits behind main
  • I linked the correct issue in the ### Fixed Issues section above
  • I wrote clear testing steps that cover the changes made in this PR
    • I clearly indicated the environment tests should be run in (Staging vs Production)
  • I wrote testing steps that cover success & fail scenarios (if applicable)
  • I included screenshots or videos for tests on all platforms
  • I ran the tests & verified they passed on:
    • iOS / native
    • Android / native
    • iOS / Safari
    • Android / Chrome
    • MacOS / Chrome
    • MacOS / Desktop
  • I verified there are no console errors related to changes in this PR
  • I followed proper code patterns (see Reviewing the code)
    • I added comments when the code was not self explanatory
    • I put all copy / text shown in the product in all src/languages/* files (if applicable)
    • I followed proper naming convention for platform-specific files (if applicable)
    • I followed style guidelines (in Styling.md) for all style edits I made
    • I followed the JSDocs style guidelines (in STYLE.md)
  • I followed the guidelines as stated in the Review Guidelines
  • I tested other components that can be impacted by my changes (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar are working as expected)
  • I corroborated the UI performance was not affected (the performance is the same than main branch)
  • If I created a new component I verified that a similar component doesn't exist in the codebase

PR Reviewer Checklist

  • I verified the PR has a small number of commits behind main
  • I verified the correct issue is linked in the ### Fixed Issues section above
  • I verified testing steps are clear and they cover the changes made in this PR
    • I verified the testing environment is mentioned in the test steps
  • I verified testing steps cover success & fail scenarios (if applicable)
  • I checked that screenshots or videos are included for tests on all platforms
  • I verified tests pass on all platforms & I tested again on:
    • iOS / native
    • Android / native
    • iOS / Safari
    • Android / Chrome
    • MacOS / Chrome
    • MacOS / Desktop
  • I verified there are no console errors related to changes in this PR
  • I verified proper code patterns were followed (see Reviewing the code)
    • I verified comments were added when the code was not self explanatory
    • I verified any copy / text shown in the product was added in all src/languages/* files (if applicable)
    • I verified proper naming convention for platform-specific files was followed (if applicable)
    • I verified style guidelines were followed
    • I verified the JSDocs style guidelines (in STYLE.md) were followed
  • I verified that this PR follows the guidelines as stated in the Review Guidelines
  • I verified other components are not impacted by changes in this PR (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar are working as expected)
  • I verified the UI performance was not affected (the performance is the same than main branch)
  • If a new component is created I verified that a similar component doesn't exist in the codebase

QA Steps

  1. Go to workspace > Connect bank account > Connect manually
  2. Go to company information page
  3. Check the fields with max character limit (ex:- SSN, zip code, phone number field) don't have any kind of character counter below when you type something.
  • Verify that no errors appear in the JS console

Screenshots

Web

Web

Mobile Web

mWeb (1)

Desktop

Desktop (1)

iOS

ios

Android

Android (1)

@Tushu17 Tushu17 requested a review from a team as a code owner March 14, 2022 21:31
@MelvinBot MelvinBot requested review from mountiny and rushatgabhane and removed request for a team March 14, 2022 21:31
@rushatgabhane
Copy link
Member

rushatgabhane commented Mar 14, 2022 •
edited
Loading

@Tushu17 please add tests, and elaborate them. It's fine to overexplain the steps without making any assumptions
Thanks so much!

rushatgabhane
rushatgabhane reviewed Mar 14, 2022
View reviewed changes
src/components/TextInput/BaseTextInput.js Outdated
</Text>
)}
</View>
{!_.isEmpty(this.props.errorText) && (
Copy link
Member

@rushatgabhane rushatgabhane Mar 14, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

@Tushu17 correct me if I'm wrong, but is it really necessary to perform this check?
Because InlineErrorText is doing this check for us, and returning null :)

App/src/components/InlineErrorText.js

Lines 20 to 23 in c1bb574

const InlineErrorText = (props) => {
if (_.isEmpty(props.children)) {
return null;
}

Copy link
Contributor Author

@Tushu17 Tushu17 Mar 15, 2022

Choose a reason for hiding this comment

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

Yeah we don't need it, but I checked other places and that's how we had implemented this. Should I change it?

Copy link
Member

@rushatgabhane rushatgabhane Mar 15, 2022

Choose a reason for hiding this comment

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

Yes, I don't see why not

mountiny
mountiny reviewed Mar 15, 2022
View reviewed changes
Copy link
Contributor

@mountiny mountiny left a comment

Choose a reason for hiding this comment

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

Thanks @Tushu17.

Can you please confirm, that if user inputs too many characters OR they hit the limit (not sure now if they will be able to input more than max, probably not), is there some indication of this? This is not a problem now, just wanted to ask (ie does it inform the user max length has been used)

Thanks!

@Tushu17
Copy link
Contributor Author

Tushu17 commented Mar 15, 2022

Can you please confirm, that if user inputs too many characters OR they hit the limit (not sure now if they will be able to input more than max, probably not), is there some indication of this? This is not a problem now, just wanted to ask (ie does it inform the user max length has been used)

When the user hits the limit we don't allow adding more characters, so the user will know that they have reached the max character limit.

@mountiny
Copy link
Contributor

mountiny commented Mar 15, 2022

When the user hits the limit we don't allow adding more characters, so the user will know that they have reached the max character limit.

Yeah, maybe it might be better for UX to indicate to the user somehow this is the case, but we will see in future if this will be a problem actually.

Thanks!

@rushatgabhane
Copy link
Member

rushatgabhane commented Mar 15, 2022

yeah, it can be frustrating if you don't realize that only last 4 digits of SSN is being asked. Typing more than that does nothing

@mountiny
Copy link
Contributor

mountiny commented Mar 15, 2022

The placeholder at least says it so I would not worry about that one so much, but if there are some other fields, where the limit is not completely intuitive, it could be problematic, but I would not worry about it now and time will tell if it is actual problem and we can fix it later!

rushatgabhane
rushatgabhane suggested changes Mar 15, 2022
View reviewed changes
Copy link
Member

@rushatgabhane rushatgabhane left a comment

Choose a reason for hiding this comment

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

@Tushu17 we also need to remove the default error message. It was originally added for testing purposes ig.

App/src/components/InlineErrorText.js

Lines 15 to 16 in 06bfc16

const defaultProps = {
children: 'Error',

@rushatgabhane
Copy link
Member

rushatgabhane commented Mar 15, 2022

After this is merged, I'll submit a clean-up PR to remove some dead code

@Tushu17
Copy link
Contributor Author

Tushu17 commented Mar 16, 2022

@Tushu17 we also need to remove the default error message. It was originally added for testing purposes ig.

Removed it.

rushatgabhane
rushatgabhane approved these changes Mar 16, 2022
View reviewed changes
Copy link
Member

@rushatgabhane rushatgabhane left a comment

Choose a reason for hiding this comment

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

@mountiny LGTM! 🎉

PR Reviewer Checklist

  • I verified the PR has a small number of commits behind main
  • I verified the correct issue is linked in the ### Fixed Issues section above
  • I verified testing steps are clear and they cover the changes made in this PR
    • I verified the testing environment is mentioned in the test steps
  • I verified testing steps cover success & fail scenarios (if applicable)
  • I checked that screenshots or videos are included for tests on all platforms
  • I verified tests pass on all platforms & I tested again on:
    • iOS / native
    • Android / native
    • iOS / Safari
    • Android / Chrome
    • MacOS / Chrome
    • MacOS / Desktop
  • I verified there are no console errors related to changes in this PR
  • I verified proper code patterns were followed (see Reviewing the code)
    • I verified comments were added when the code was not self explanatory
    • I verified any copy / text shown in the product was added in all src/languages/* files (if applicable)
    • I verified proper naming convention for platform-specific files was followed (if applicable)
    • I verified style guidelines were followed
    • I verified the JSDocs style guidelines (in STYLE.md) were followed
  • I verified that this PR follows the guidelines as stated in the Review Guidelines
  • I verified other components are not impacted by changes in this PR (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar are working as expected)
  • I verified the UI performance was not affected (the performance is the same than main branch)
  • If a new component is created I verified that a similar component doesn't exist in the codebase

mountiny
mountiny reviewed Mar 16, 2022
View reviewed changes
Copy link
Contributor

@mountiny mountiny left a comment

Choose a reason for hiding this comment

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

Looks good! Just gonna wait for Rushat to test this one out! Thank you very much!

@mountiny
Copy link
Contributor

mountiny commented Mar 16, 2022

image

@mountiny mountiny self-requested a review March 16, 2022 19:52
mountiny
mountiny approved these changes Mar 16, 2022
View reviewed changes
Copy link
Contributor

@mountiny mountiny left a comment

Choose a reason for hiding this comment

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

@rushatgabhane @Tushu17 Thank you very much!

@mountiny mountiny merged commit c30e95f into Expensify:main Mar 16, 2022
@OSBotify
Copy link
Contributor

OSBotify commented Mar 16, 2022

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

@rushatgabhane rushatgabhane mentioned this pull request Mar 17, 2022
Merged
64 tasks
@OSBotify OSBotify mentioned this pull request Mar 17, 2022
Closed
31 tasks
@OSBotify
Copy link
Contributor

OSBotify commented Mar 17, 2022

🚀 Deployed to staging by @mountiny in version: 1.1.44-0 🚀

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

@parasharrajat parasharrajat mentioned this pull request Mar 18, 2022
Closed
@rushatgabhane rushatgabhane mentioned this pull request Mar 19, 2022
Merged
86 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mountiny mountiny mountiny approved these changes

+1 more reviewer

@rushatgabhane rushatgabhane rushatgabhane approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@Tushu17 @rushatgabhane @mountiny @OSBotify