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

[No QA] Clarify some language in the CONTRIBUTING.md #14989

Merged
merged 1 commit into from
Feb 9, 2023

Conversation

marcaaron
Copy link
Contributor

Details

I have noticed a lot of similar proposals get posted. People are basically just ripping off other ideas. New proposals should be different. I think maybe not everyone understood what the word "substantive" means.

Fixed Issues

N/A

Tests

None

  • Verify that no errors appear in the JS console

Offline tests

None

QA Steps

None

  • 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.
  • I have checked off every checkbox in the PR author checklist, including those that don't apply to this PR.

Screenshots/Videos

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

@marcaaron marcaaron requested a review from a team as a code owner February 9, 2023 16:52
@marcaaron marcaaron self-assigned this Feb 9, 2023
@melvin-bot melvin-bot bot requested review from grgia and removed request for a team February 9, 2023 16:53
@MelvinBot
Copy link

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

@grgia grgia merged commit 68d7b93 into main Feb 9, 2023
@grgia grgia deleted the marcaaron-fixSubstantive branch February 9, 2023 17:22
@grgia
Copy link
Contributor

grgia commented Feb 9, 2023

Merged without the checklist passing because I clicked it too fast. I think it's fine since it's a README change

@OSBotify
Copy link
Contributor

OSBotify commented Feb 9, 2023

✋ 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

github-actions bot commented Feb 9, 2023

Performance Comparison Report 📊

Significant Changes To Duration

There are no entries

Meaningless Changes To Duration

Show entries
Name Duration
App start TTI 708.055 ms → 720.484 ms (+12.429 ms, +1.8%)
App start runJsBundle 195.063 ms → 201.875 ms (+6.813 ms, +3.5%)
App start nativeLaunch 19.800 ms → 20.375 ms (+0.575 ms, +2.9%)
App start regularAppStart 0.014 ms → 0.015 ms (+0.001 ms, +4.6%)
Open Search Page TTI 612.430 ms → 604.363 ms (-8.067 ms, -1.3%)
Show details
Name Duration
App start TTI Baseline
Mean: 708.055 ms
Stdev: 34.139 ms (4.8%)
Runs: 658.4197690000292 658.6795409999322 666.7023279999848 667.5020429999568 669.8840600000694 671.4883580000605 676.8991930000484 682.9150459999219 683.4345289999619 685.873692000052 688.5358859999105 691.7354389999527 692.3503630000632 700.1282579998951 700.3658620000351 700.6979179999325 701.7245120001025 701.7583600000944 702.4128950000741 714.2104629999958 714.4889919999987 715.4488230000716 722.1760150000919 724.9585049999878 741.2228880000766 742.723624000093 745.5597469999921 750.4499550000764 762.9422130000312 763.3569050000515 779.1729530000594 779.5319570000283

Current
Mean: 720.484 ms
Stdev: 29.404 ms (4.1%)
Runs: 673.4092629998922 681.8071699999273 683.9404140000697 683.9817500000354 684.2930950000882 687.4130390000064 690.674291000003 693.8965239999816 696.0267529999837 699.3031059999485 702.7819729999173 702.8649869998917 705.609269999899 710.7288909999188 710.8754690000787 714.3612130000256 721.3199740001 726.1754739999305 726.8072220000904 727.616594000021 734.9272320000455 739.7128459999803 740.0226479999255 740.5342009998858 743.0266040000133 744.0963020001072 744.5018420000561 756.7380520000588 765.817732000025 769.4808539999649 772.7391900001094 779.9979479999747
App start runJsBundle Baseline
Mean: 195.063 ms
Stdev: 23.451 ms (12.0%)
Runs: 164 165 168 170 171 172 175 175 176 176 180 182 183 185 191 192 192 195 196 198 199 200 201 202 205 217 217 229 235 241 243 247

Current
Mean: 201.875 ms
Stdev: 22.729 ms (11.3%)
Runs: 163 169 173 180 180 180 181 183 184 187 187 190 193 193 193 194 195 197 198 210 211 211 219 220 221 225 228 233 234 237 238 253
App start nativeLaunch Baseline
Mean: 19.800 ms
Stdev: 1.579 ms (8.0%)
Runs: 17 18 18 18 18 18 18 19 19 19 19 19 19 20 20 20 20 20 20 20 20 20 21 21 21 21 22 22 23 24

Current
Mean: 20.375 ms
Stdev: 1.850 ms (9.1%)
Runs: 18 18 18 18 19 19 19 19 19 19 19 19 20 20 20 20 20 20 20 20 20 21 21 22 22 22 22 22 23 23 25 25
App start regularAppStart Baseline
Mean: 0.014 ms
Stdev: 0.001 ms (6.2%)
Runs: 0.012776999967172742 0.012981000123545527 0.013184000039473176 0.013264999957755208 0.013427000027149916 0.013428000034764409 0.013590999878942966 0.013753000181168318 0.014079000102356076 0.014159999787807465 0.014160000020638108 0.01424099993892014 0.014282000018283725 0.014321999857202172 0.01432300009764731 0.01432300009764731 0.01432300009764731 0.01448600017465651 0.014811999863013625 0.0148930000141263 0.014973999932408333 0.015015000011771917 0.015055000083521008 0.015176999848335981 0.01525900000706315 0.015299000078812242 0.015300000086426735 0.015422000084072351 0.015625 0.0157880000770092 0.01664300006814301

Current
Mean: 0.015 ms
Stdev: 0.001 ms (6.7%)
Runs: 0.013143000192940235 0.013671000022441149 0.013956999871879816 0.01395700010471046 0.014038000022992492 0.01407799986191094 0.014118999941274524 0.014159999787807465 0.014607000164687634 0.014688999857753515 0.0147299999371171 0.014771000016480684 0.014771000016480684 0.014851999934762716 0.014851999934762716 0.014974000165238976 0.015055000083521008 0.01509599993005395 0.01513600000180304 0.01525900000706315 0.015381000004708767 0.015462999930605292 0.0157880000770092 0.016072999918833375 0.01619499991647899 0.01647900021634996 0.016521000070497394 0.016521000070497394 0.01676399982534349 0.016887000063434243 0.017048999899998307
Open Search Page TTI Baseline
Mean: 612.430 ms
Stdev: 20.502 ms (3.3%)
Runs: 573.5665700000245 589.1735030000564 589.3588870000094 590.1241460000165 591.5056570000015 593.6445730000269 594.5320230000652 597.4344490000512 598.7159019999672 600.5340980000328 600.9511309999507 603.3984380001202 603.6066079998855 603.8651130001526 605.046061999863 605.668864000123 606.9231370000634 609.9834399998654 609.9977619999554 611.053549000062 613.0600590000395 616.0465899999253 619.3729659998789 619.6433109999634 624.3347569999751 624.8605550001375 629.4324550000019 637.2021889998578 638.1985280001536 639.3299970000517 649.4418940001633 659.1863210001029 660.9939379999414

Current
Mean: 604.363 ms
Stdev: 17.867 ms (3.0%)
Runs: 568.779663000023 569.582112000091 577.593057999853 577.6116949999705 583.3023279998451 584.5608730001841 584.9505209999625 589.9198000000324 591.5698250001296 595.4754640001338 600.0869549999479 601.7932539999019 602.0762529999483 602.1170659998897 603.599040000001 603.6536459999625 603.730916999979 606.3656830000691 606.9286299999803 611.7012120001018 611.9786790001672 612.0901689999737 614.5339359999634 615.6901849999558 615.9036870000418 616.5931810000911 618.8097739999648 619.7657469999976 621.8659270000644 622.6102700000629 628.3752850000747 639.2664799999911 641.097453000024

@OSBotify
Copy link
Contributor

🚀 Deployed to staging by https://github.com/grgia in version: 1.2.69-0 🚀

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

@OSBotify
Copy link
Contributor

🚀 Deployed to production by https://github.com/mountiny in version: 1.2.69-2 🚀

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