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.

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

year restriction to 4 digit in date inputs #14272

Merged
merged 1 commit into from
Jan 13, 2023

Conversation

Pujan92
Copy link
Contributor

@Pujan92 Pujan92 commented Jan 13, 2023

@yuwenmemon @mollfpr

Details

Restrict year to 4 digits in the date inputs

  • Used max property to provide max date for the input which won't allow more than 4 digits in year -- Works in Chrome
  • Passed value true to moment constructor parameter strict which considers 4 digit in a year to be valid -- Partially works in Safari & Firefox

Fixed Issues

$ #13837
PROPOSAL: #13837 (comment)

Tests

  1. Open any datepicker in the application
  2. Verify when we reach to year part it won't allow more than 4 digits(This needs to test on Web and desktop only, for mWeb and native platforms just need to test datepicker working normally or not)
  • Verify that no errors appear in the JS console

Offline tests

QA Steps

  1. Open any datepicker in the application
  2. Verify when we reach to year part it won't allow more than 4 digits(This needs to test on Web and desktop only, for mWeb and native platforms just need to test datepicker working normally or not)
  • Verify that no errors appear in the JS console

PR Author Checklist

  • 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 added steps for local testing in the Tests section
    • I added steps for the expected offline behavior in the Offline steps section
    • I added steps for Staging and/or Production testing in the QA steps section
    • I added steps to cover failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
    • I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
    • I tested this PR with a High Traffic account against the staging or production API to ensure there are no regressions (e.g. long loading states that impact usability).
  • I included screenshots or videos for tests on all platforms
  • I ran the tests on all platforms & verified they passed on:
    • Android / native
    • Android / Chrome
    • iOS / native
    • iOS / Safari
    • MacOS / Chrome / Safari
    • MacOS / Desktop
  • I verified there are no console errors (if there's a console error not related to the PR, report it or open an issue for it to be fixed)
  • I followed proper code patterns (see Reviewing the code)
    • I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick)
    • I verified that comments were added to code that is not self explanatory
    • I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
    • I verified any copy / text shown in the product is localized by adding it to src/languages/* files and using the translation method
    • I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
    • 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.
    • I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
    • I verified the JSDocs style guidelines (in STYLE.md) were followed
  • If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
  • 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 verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
  • I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
  • I verified that if a function's arguments changed that all usages have also been updated correctly
  • If a new component is created I verified that:
    • A similar component doesn't exist in the codebase
    • All props are defined accurately and each prop has a /** comment above it */
    • The file is named correctly
    • The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
    • The only data being stored in the state is data necessary for rendering and nothing else
    • For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
    • Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
    • All JSX used for rendering exists in the render method
    • The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
  • If any new file was added I verified that:
    • The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
  • If a new CSS style is added I verified that:
    • A similar style doesn't already exist
    • The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
  • If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
  • If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
  • If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
  • I have checked off every checkbox in the PR author checklist, including those that don't apply to this PR.

Screenshots/Videos

Web
Screen.Recording.2023-01-13.at.2.51.43.PM.mov
Mobile Web - Chrome Screenshot 2023-01-13 at 3 02 34 PM
Mobile Web - Safari Screenshot 2023-01-13 at 2 56 59 PM
Desktop
Screen.Recording.2023-01-13.at.3.07.30.PM.mov
iOS

Simulator Screen Shot - iPhone 14 - 2023-01-13 at 14 58 52

Android

Screenshot_1673602514

@Pujan92 Pujan92 requested a review from a team as a code owner January 13, 2023 09:38
@melvin-bot
Copy link

melvin-bot bot commented Jan 13, 2023

Hey! I see that you made changes to our Form component. Make sure to update the docs in FORMS.md accordingly. Cheers!

@melvin-bot melvin-bot bot requested review from Gonals and removed request for a team January 13, 2023 09:39
@melvin-bot
Copy link

melvin-bot bot commented Jan 13, 2023

@Gonals 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]

@Pujan92
Copy link
Contributor Author

Pujan92 commented Jan 13, 2023

I think the mistake I made in GitHub link while raising the PR caused @yuwenmemon @mollfpr not taken automatically as a reviewer.

@mollfpr
Copy link
Contributor

mollfpr commented Jan 13, 2023

Reviewer Checklist

  • I have verified the author checklist is complete (all boxes are checked off).
  • 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 steps for local testing are in the Tests section
    • I verified the steps for Staging and/or Production testing are in the QA steps section
    • I verified the steps cover any possible failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
    • I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
  • I checked that screenshots or videos are included for tests on all platforms
  • I included screenshots or videos for tests on all platforms
  • I verified tests pass on all platforms & I tested again on:
    • Android / native
    • Android / Chrome
    • iOS / native
    • iOS / Safari
    • MacOS / Chrome / Safari
    • MacOS / Desktop
  • If there are any errors in the console that are unrelated to this PR, I either fixed them (preferred) or linked to where I reported them in Slack
  • I verified proper code patterns were followed (see Reviewing the code)
    • I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick).
    • I verified that comments were added to code that is not self explanatory
    • I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
    • I verified any copy / text shown in the product is localized by adding it to src/languages/* files and using the translation method
    • I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
    • 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.
    • I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
    • I verified the JSDocs style guidelines (in STYLE.md) were followed
  • If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
  • I verified that this PR follows the guidelines as stated in the Review Guidelines
  • I verified other components that can be impacted by these changes have been tested, and I retested again (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar have been tested & I retested again)
  • I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
  • I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
  • If a new component is created I verified that:
    • A similar component doesn't exist in the codebase
    • All props are defined accurately and each prop has a /** comment above it */
    • The file is named correctly
    • The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
    • The only data being stored in the state is data necessary for rendering and nothing else
    • For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
    • Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
    • All JSX used for rendering exists in the render method
    • The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
  • If any new file was added I verified that:
    • The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
  • If a new CSS style is added I verified that:
    • A similar style doesn't already exist
    • The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
  • If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
  • If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
  • If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
  • I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

Screenshots/Videos

Web
14272.Web-Safari.mov
14272.Web-Chrome.mov
Mobile Web - Chrome
14272.mWeb-Chrome.mov
Mobile Web - Safari
14272.mWeb-Safari.mov
Desktop
14272.Desktop.mov
iOS
14272.iOS.mov
Android
14272.Android.mov

Copy link
Contributor

@mollfpr mollfpr left a comment

Choose a reason for hiding this comment

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

LGTM 👍

Just need add clarity to the test case.

  1. Open any datepicker in the application
  2. Verify when we reach to year part it won't allow more than 4 digits

@Pujan92 We should specify that the test is only on Web and Desktop. For mWeb and the native platforms just need to verify that the date picker is still working normally.

@mollfpr
Copy link
Contributor

mollfpr commented Jan 13, 2023

All yours @yuwenmemon

@yuwenmemon yuwenmemon self-requested a review January 13, 2023 19:45
@yuwenmemon yuwenmemon merged commit 405d40d into Expensify:main Jan 13, 2023
@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.

@github-actions
Copy link
Contributor

Performance Comparison Report 📊

Significant Changes To Duration

There are no entries

Meaningless Changes To Duration

Show entries
Name Duration
App start TTI 658.795 ms → 686.575 ms (+27.780 ms, +4.2%)
App start runJsBundle 179.125 ms → 191.355 ms (+12.230 ms, +6.8%)
App start nativeLaunch 19.484 ms → 19.839 ms (+0.355 ms, +1.8%)
App start regularAppStart 0.020 ms → 0.014 ms (-0.005 ms, -27.4%) 🟢
Open Search Page TTI 606.765 ms → 605.818 ms (-0.947 ms, ±0.0%)
Show details
Name Duration
App start TTI Baseline
Mean: 658.795 ms
Stdev: 26.119 ms (4.0%)
Runs: 621.9797219997272 624.2704020002857 624.5484929997474 625.4288050001487 627.2758170003071 632.0224959999323 634.1105960002169 634.6318260002881 634.8758619995788 635.3341680001467 636.8050180003047 639.8886160003021 641.4059389997274 648.8083870001137 655.6421590000391 658.5972119998187 659.3292960003018 661.6544479997829 664.9024470001459 670.462570999749 670.8706959998235 672.3312100004405 679.924635999836 680.2439639996737 681.7676200000569 685.8856030004099 685.885819000192 686.01948300004 694.4460840001702 698.084746000357 699.8454290004447 714.1456469995901

Current
Mean: 686.575 ms
Stdev: 33.452 ms (4.9%)
Runs: 627.7658689999953 633.8546820003539 639.4045719997957 654.6838309997693 655.1324319997802 656.3622179999948 657.9365130001679 663.7847300004214 664.0375650003552 664.18904799968 665.0533670000732 666.3477469999343 666.5118190003559 672.509890999645 677.7182759996504 681.3864620001987 688.2654609996825 689.0695770001039 691.0542940003797 691.119036000222 696.8050389997661 704.1407180000097 706.1509079998359 707.2579819997773 709.1178949996829 710.2070770002902 713.8937499998137 724.1177000002936 728.8725880002603 739.8582699997351 757.9860380003229 765.8006920004264
App start runJsBundle Baseline
Mean: 179.125 ms
Stdev: 19.877 ms (11.1%)
Runs: 151 152 159 160 161 161 162 162 164 164 165 167 169 170 170 171 173 174 178 180 182 184 188 198 201 202 202 204 206 207 219 226

Current
Mean: 191.355 ms
Stdev: 18.105 ms (9.5%)
Runs: 159 160 163 166 174 177 177 179 182 182 185 185 185 188 189 191 191 191 192 198 198 200 201 202 206 211 211 213 217 222 237
App start nativeLaunch Baseline
Mean: 19.484 ms
Stdev: 2.030 ms (10.4%)
Runs: 17 17 18 18 18 18 18 18 18 18 18 18 18 18 18 19 19 19 20 20 20 20 20 20 21 21 22 23 24 24 24

Current
Mean: 19.839 ms
Stdev: 1.868 ms (9.4%)
Runs: 17 17 17 18 18 18 18 19 19 19 19 19 19 19 19 19 19 20 20 20 20 21 21 22 22 22 22 22 23 23 24
App start regularAppStart Baseline
Mean: 0.020 ms
Stdev: 0.002 ms (8.0%)
Runs: 0.016885999590158463 0.018350999802350998 0.018352000042796135 0.018432999961078167 0.01855500042438507 0.018797999247908592 0.018880000337958336 0.018962000496685505 0.01912499964237213 0.019164999946951866 0.0192050002515316 0.019206000491976738 0.019247000105679035 0.019328000023961067 0.0194089999422431 0.019531000405550003 0.01969399955123663 0.02001900039613247 0.0204670000821352 0.0206300001591444 0.02071200031787157 0.020793000236153603 0.020793000236153603 0.0208339998498559 0.020874000154435635 0.02176899928599596 0.022094999440014362 0.023599999956786633 0.024577000178396702

Current
Mean: 0.014 ms
Stdev: 0.001 ms (4.1%)
Runs: 0.013143000192940235 0.013469000346958637 0.013753000646829605 0.013794000260531902 0.013794000260531902 0.013875000178813934 0.01403799932450056 0.014078999869525433 0.014241000637412071 0.014281999319791794 0.014403999783098698 0.014405000023543835 0.01444500032812357 0.014525999315083027 0.014607999473810196 0.01464799977838993 0.014729000627994537 0.014810999855399132 0.0148930000141263 0.0148930000141263 0.014933000318706036 0.014933999627828598 0.015015000477433205 0.015137000009417534 0.015177999623119831 0.01529999915510416 0.01570700015872717
Open Search Page TTI Baseline
Mean: 606.765 ms
Stdev: 23.127 ms (3.8%)
Runs: 563.8699949998409 566.5535479998216 572.2150480002165 579.0861419998109 582.0365810003132 585.1495770001784 587.1631269995123 587.3385419994593 591.5770680001006 593.4637050004676 593.8972979998216 596.7068689996377 599.2207439998165 599.7796630002558 603.8201500000432 606.9466150002554 608.0264900000766 609.1827389998361 609.6875409996137 610.9676109999418 613.0573740005493 615.7617190005258 616.2737220004201 616.8717040000483 618.2738449992612 618.7875169999897 623.1857100008056 624.4093429995701 632.0087900003418 633.3869629995897 647.3149009998888 654.4767660005018 662.7440600004047

Current
Mean: 605.818 ms
Stdev: 24.690 ms (4.1%)
Runs: 559.5952150002122 570.5904540000483 571.613119000569 578.1872970005497 579.3235679995269 580.3787850001827 582.2139900000766 587.0296229999512 588.0717369997874 594.0225420007482 594.8801279999316 595.4564619995654 595.6636159997433 596.0537109998986 596.6642260001972 600.2499599996954 600.9146330002695 604.2112640002742 609.2147220000625 611.5464279996231 612.6693119993433 613.1612140005454 613.5065509993583 616.2111009992659 618.7567549999803 621.3736979998648 625.2860510004684 634.3022460006177 635.7556159999222 640.650308999233 643.771972999908 652.5620939992368 668.0992029998451

@OSBotify
Copy link
Contributor

🚀 Deployed to staging by @yuwenmemon in version: 1.2.55-0 🚀

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

@OSBotify
Copy link
Contributor

🚀 Deployed to production by @AndrewGable in version: 1.2.55-0 🚀

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants