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

Date format placeholder is missing for Date field on the date of birth page #15344

Merged
merged 1 commit into from
Feb 22, 2023

Conversation

narefyev91
Copy link
Contributor

@narefyev91 narefyev91 commented Feb 22, 2023

Details

For native apps missed some required props to show default placeholder

Fixed Issues

$ #15119
$ #15119 (comment)

Tests

  1. Only on native iOS and Android
  2. Log in with an account for which the date of birth has never been set.
  3. Navigate to Settings > Profile > Personal Details->Date of Birth.
  • Verify that no errors appear in the JS console

Offline tests

This feature doesn't change or is impacted by offline mode.

QA Steps

Same as above.

  • 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
      • If any non-english text was added/modified, I verified the translation was requested/reviewed in #expensify-open-source and it was approved by an internal Expensify engineer. Link to Slack message:
    • 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.
  • If the main branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to the Test steps.
  • I have checked off every checkbox in the PR author checklist, including those that don't apply to this PR.

Screenshots/Videos

Web Screenshot 2023-02-22 at 14 57 54
Mobile Web - Chrome

Screenshot_20230222_150757

Mobile Web - Safari

Simulator Screen Shot - iPhone 14 Pro - 2023-02-22 at 14 59 53

Desktop Screenshot 2023-02-22 at 15 11 30
iOS

Simulator Screen Shot - iPhone 14 Pro - 2023-02-22 at 13 28 20

Android

Screenshot_20230222_133713

@narefyev91 narefyev91 requested a review from a team as a code owner February 22, 2023 12:21
@MelvinBot
Copy link

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 mountiny and thesahindia and removed request for a team February 22, 2023 12:21
@MelvinBot
Copy link

@mountiny @thesahindia One of you needs to 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]

@mountiny
Copy link
Contributor

@narefyev91 Can you also please go through the other platforms and show how this looks there? To fill in all the required fields in the PR author checklist. Thanks

@mountiny mountiny removed the request for review from thesahindia February 22, 2023 12:53
@mountiny
Copy link
Contributor

@thesahindia I think no review from you is required here, thanks!

@narefyev91
Copy link
Contributor Author

@narefyev91 Can you also please go through the other platforms and show how this looks there? To fill in all the required fields in the PR author checklist. Thanks

@mountiny added screenshots

@mountiny
Copy link
Contributor

mountiny commented Feb 22, 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.
  • If the main branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to the Test steps.
  • I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

Screenshots/Videos

Web image
Mobile Web - Chrome image
Mobile Web - Safari image
Desktop image
iOS image
Android image

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.

The native apps test well and I have not seen any regression on the other platforms, its not ideal that the placeholder is not consistent but I assume this is just work in progress and we will slowly get to an ideal state as we add our own component and do some polish over there

@mountiny
Copy link
Contributor

Because there are no real objections I will go ahead and merge this PR. thanks @narefyev91

@mountiny mountiny merged commit d807241 into Expensify:main Feb 22, 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 746.938 ms → 788.296 ms (+41.358 ms, +5.5%)
App start runJsBundle 202.276 ms → 220.258 ms (+17.982 ms, +8.9%)
Open Search Page TTI 594.287 ms → 595.168 ms (+0.882 ms, ±0.0%)
App start nativeLaunch 20.094 ms → 20.207 ms (+0.113 ms, +0.6%)
App start regularAppStart 0.016 ms → 0.016 ms (+0.000 ms, +0.6%)
Show details
Name Duration
App start TTI Baseline
Mean: 746.938 ms
Stdev: 18.592 ms (2.5%)
Runs: 714.9658129997551 715.6393949985504 716.4137849994004 721.5026509985328 721.645844001323 723.110814999789 731.2509180009365 736.7872189991176 740.0970650017262 740.4788680002093 741.1232569999993 742.7466880008578 746.0556450001895 746.9595559984446 750.7932289987803 751.172171998769 751.4549250006676 752.1827690005302 752.4724120013416 754.6451929993927 758.7627540007234 758.9252570010722 762.9847990013659 763.6931690014899 764.5924459993839 765.39631799981 770.9513380005956 777.8259360007942 786.5693559981883

Current
Mean: 788.296 ms
Stdev: 31.463 ms (4.0%)
Runs: 735.2841610014439 746.4066749997437 750.399946000427 754.1765550002456 755.43857299909 756.2687110006809 759.7062710002065 760.7820679992437 765.3072180002928 770.019622001797 771.2925469987094 775.0643430016935 776.3730689994991 783.8941479995847 785.6581810005009 787.7053829990327 791.6398690007627 792.7781089991331 793.5860660001636 795.212228000164 802.1366240009665 804.4249509982765 807.7870160005987 811.8908189982176 812.4175410009921 821.3420789986849 824.5707090012729 839.9986030012369 846.4760700017214 870.8475189991295
App start runJsBundle Baseline
Mean: 202.276 ms
Stdev: 12.066 ms (6.0%)
Runs: 182 184 184 185 190 192 194 196 196 196 197 198 200 200 201 202 202 203 205 206 209 210 213 215 215 215 223 224 229

Current
Mean: 220.258 ms
Stdev: 18.719 ms (8.5%)
Runs: 192 197 200 200 200 203 203 205 206 208 209 209 212 214 215 217 218 220 226 226 227 227 229 229 231 231 247 252 253 256 266
Open Search Page TTI Baseline
Mean: 594.287 ms
Stdev: 26.535 ms (4.5%)
Runs: 555.6146240010858 555.6796070002019 556.8203939981759 560.3885909989476 565.8659269995987 568.4771729968488 573.2736820019782 574.593913000077 576.2296960018575 577.3098959997296 579.1037600003183 581.7806809991598 582.225464001298 583.0644529983401 590.212320998311 592.7428390011191 594.5386960022151 595.2689619995654 595.8625079989433 598.8200690001249 602.9707040004432 603.7294520027936 604.0935879983008 604.8210450001061 606.4614669978619 619.3743089996278 619.7565109990537 621.4331470020115 627.3861900009215 632.8596199974418 651.7471120022237 664.6665860004723

Current
Mean: 595.168 ms
Stdev: 25.247 ms (4.2%)
Runs: 557.4329430013895 559.6770029999316 565.0776779986918 566.306477997452 567.4046630002558 568.6589360013604 571.8070069998503 574.9285070002079 576.6905930005014 579.2591149993241 581.6090500019491 581.6737879998982 583.2563080005348 584.8414310030639 590.4937340021133 591.3320729993284 593.8899749964476 594.1490480005741 595.1548669971526 595.819947000593 599.023520000279 611.312215000391 611.5816649980843 613.8786619976163 614.0742999985814 614.5190020017326 615.8679200001061 623.0329999998212 624.0329589992762 626.9110510013998 644.3918460011482 667.2956949993968
App start nativeLaunch Baseline
Mean: 20.094 ms
Stdev: 1.926 ms (9.6%)
Runs: 17 18 18 18 18 18 19 19 19 19 19 19 19 19 19 19 20 20 20 20 20 20 21 22 22 22 22 22 23 23 24 25

Current
Mean: 20.207 ms
Stdev: 1.399 ms (6.9%)
Runs: 18 18 18 19 19 19 19 19 19 20 20 20 20 20 20 20 20 20 20 20 21 21 22 22 22 22 22 23 23
App start regularAppStart Baseline
Mean: 0.016 ms
Stdev: 0.001 ms (7.9%)
Runs: 0.01314299926161766 0.013997998088598251 0.014038000255823135 0.01489199697971344 0.014973998069763184 0.015055999159812927 0.015177000313997269 0.01521800085902214 0.015381000936031342 0.015461999922990799 0.015706002712249756 0.0157880000770092 0.0157880000770092 0.015829000622034073 0.015990998595952988 0.01607299968600273 0.016234997659921646 0.01631699874997139 0.01631699874997139 0.016398001462221146 0.016641996800899506 0.016885999590158463 0.01688600331544876 0.01700800284743309 0.017292998731136322 0.01729400083422661 0.017537999898195267 0.017618998885154724 0.019694000482559204

Current
Mean: 0.016 ms
Stdev: 0.001 ms (4.9%)
Runs: 0.014242000877857208 0.01476999744772911 0.014851998537778854 0.015014998614788055 0.015381000936031342 0.015461999922990799 0.015502996742725372 0.01550300046801567 0.015542998909950256 0.015707001090049744 0.01590999960899353 0.015990998595952988 0.01603199914097786 0.016114000231027603 0.01615399867296219 0.016194000840187073 0.016276001930236816 0.016357000917196274 0.016357000917196274 0.016398001462221146 0.016398001462221146 0.016479000449180603 0.016561001539230347 0.016561001539230347 0.016683001071214676 0.016724001616239548 0.017048999667167664 0.01713000237941742 0.017131000757217407 0.01729300245642662 0.01782200112938881

@OSBotify
Copy link
Contributor

🚀 Deployed to staging by https://github.com/mountiny in version: 1.2.76-0 🚀

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

@OSBotify
Copy link
Contributor

🚀 Deployed to production by https://github.com/yuwenmemon in version: 1.2.76-7 🚀

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

1 similar comment
@OSBotify
Copy link
Contributor

🚀 Deployed to production by https://github.com/yuwenmemon in version: 1.2.76-7 🚀

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

@OSBotify
Copy link
Contributor

🚀 Deployed to production by https://github.com/yuwenmemon in version: 1.2.76-7 🚀

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.

6 participants