From 71de239175a66a28beb167f45d1c28102639012e Mon Sep 17 00:00:00 2001 From: Tim Golen Date: Mon, 4 Jul 2022 13:40:29 -0600 Subject: [PATCH 01/11] Move forms document --- FORMS.md | 214 +------------------------------------------------- docs/FORMS.md | 213 +++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 214 insertions(+), 213 deletions(-) create mode 100644 docs/FORMS.md diff --git a/FORMS.md b/FORMS.md index d80b2dff3310..c6d36ff480be 100644 --- a/FORMS.md +++ b/FORMS.md @@ -1,213 +1 @@ -# Creating and Using Forms - -This document lists specific guidelines for using our Form component and general forms guidelines. - -## General Form UI/UX - -### Labels, Placeholders, & Hints - -Labels are required for each input and should clearly mark the field. Optional text may appear below a field when a hint, suggestion, or context feels necessary. If validation fails on such a field, its error should clearly explain why without relying on the hint. Inline errors should always replace the microcopy hints. Placeholders should not be used as it’s customary for labels to appear inside form fields and animate them above the field when focused. - -![hint](https://user-images.githubusercontent.com/22219519/156266779-72deaf42-832c-453c-a5c2-1b2073b8b3b7.png) - -Labels and hints are enabled by passing the appropriate props to each input: - -```jsx - -``` - -### Character Limits - -If a field has a character limit we should give that field a max limit. This is done by passing the maxLength prop to TextInput. - -```jsx - -``` -Note: We shouldn't place a max limit on a field if the entered value can be formatted. eg: Phone number. -The phone number can be formatted in different ways. - -- 2109400803 -- +12109400803 -- (210)-940-0803 - -### Native Keyboards - -We should always set people up for success on native platforms by enabling the best keyboard for the type of input we’re asking them to provide. See [keyboardType](https://reactnative.dev/docs/0.64/textinput#keyboardtype) in the React Native documentation. - -We have a couple of keyboard types [defined](https://github.com/Expensify/App/blob/572caa9e7cf32a2d64fe0e93d171bb05a1dfb217/src/CONST.js#L357-L360) and should be used like so: - -```jsx - -``` - -### Autofill Behavior - -Forms should autofill information whenever possible i.e. they should work with browsers and password managers auto complete features. - -As a best practice we should avoid asking for information we can get via other means e.g. asking for City, State, and Zip if we can use Google Places to gather information with the least amount of hassle to the user. - -Browsers use the name prop to autofill information into the input. Here's a [reference](https://developers.google.com/web/fundamentals/design-and-ux/input/forms#recommended_input_name_and_autocomplete_attribute_values) for available values for the name prop. - -```jsx - -``` - -### Focus and Tab Behavior - -All forms should define an order in which the inputs should be filled out, and using tab / shift + tab to navigate through the form should traverse the inputs in that order/reversed order, respectively. In most cases this can be achieved by composition, i.e. rendering the components in the correct order. If we come across a situation where composition is not enough, we can: - -1. Create a local tab index state -2. Assign a tab index to each form input -3. Add an event listener to the page/component we are creating and update the tab index state on tab/shift + tab key press -4. Set focus to the input with that tab index. - -Additionally, pressing the enter key on any focused field should submit the form. - -Note: This doesn't apply to the multiline fields. To keep the browser behavior consistent, pressing enter on the multiline should not be intercepted. It should follow the default browser behavior (such as adding a newline). - -### Modifying User Input on Change - -User input that may include optional characters (e.g. (, ), - in a phone number) should never be restricted on input, nor be modified or formatted on blur. This type of input jacking is disconcerting and makes things feel broken. - -Instead we will format and clean the user input internally before using the value (e.g. making an API request where the user will never see this transformation happen). Additionally, users should always be able to copy/paste whatever characters they want into fields. - -To give a slightly more detailed example of how this would work with phone numbers we should: - -1. Allow any character to be entered in the field. -2. On blur, strip all non-number characters (with the exception of + if the API accepts it) and validate the result against the E.164 regex pattern we use for a valid phone. This change is internal and the user should not see any changes. This should be done in the validate callback passed as a prop to Form. -3. On submit, repeat validation and submit with the clean value. - -### Form Drafts - -Form inputs will NOT store draft values by default. This is to avoid accidentely storing any sensitive information like passwords, SSN or bank account information. We need to explicitly tell each form input to save draft values by passing the shouldSaveDraft prop to the input. Saving draft values is highly desireable and we should always try to save draft values. This way when a user continues a given flow they can easily pick up right where they left off if they accidentally exited a flow. Inputs with saved draft values [will be cleared when a user logs out](https://github.com/Expensify/App/blob/aa1f0f34eeba5d761657168255a1ae9aebdbd95e/src/libs/actions/SignInRedirect.js#L52) (like most data). Additionally we should clear draft data once the form is successfully submitted by calling `Onyx.set(ONYXKEY.FORM_ID, null)` in the onSubmit callback passed to Form. - -```jsx - -``` - -## Form Validation and Error handling - -### Validate on Blur and Submit - -Each individual form field that requires validation will have its own validate test defined. When the form field loses focus (blur) we will run that validate test and show feedback. A blur on one field will not cause other fields to validate or show errors unless they have already been blurred. - -All form fields will additionally be validated when the form is submitted. Although we are validating on blur this additional step is necessary to cover edge cases where forms are auto-filled or when a form is submitted by pressing enter (i.e. there will be only a ‘submit’ event and no ‘blur’ event to hook into). - -The Form component takes care of validation internally and the only requirement is that we pass a validate callback prop. The validate callback takes in the input values as argument and should return an object with shape `{[inputID]: errorMessage}`. Here's an example for a form that has two inputs, `routingNumber` and `accountNumber`: - -```js -function validate(values) { - const errors = {}; - if (!values.routingNumber) { - errors.routingNumber = props.translate(CONST.ERRORS.ROUTING_NUMBER); - } - if (!values.accountNumber) { - errors.accountNumber = props.translate(CONST.ERRORS.ACCOUNT_NUMBER); - } - return errors; -} -``` - -For a working example, check [Form story](https://github.com/Expensify/App/blob/aa1f0f34eeba5d761657168255a1ae9aebdbd95e/src/stories/Form.stories.js#L63-L72) - -### Highlight Fields and Inline Errors - -Individual form fields should be highlighted with a red error outline and present supporting inline error text below the field. Error text will be required for all required fields and optional fields that require validation. This will keep our error handling consistent and ensure we put in a good effort to help the user fix the problem by providing more information than less. - -![error](https://user-images.githubusercontent.com/22219519/156267035-af40fe93-da27-4e16-bc55-b7cd40b0f1f2.png) - -### Multiple Types of Errors for Individual Fields - -Individual fields should support multiple messages depending on validation e.g. a date could be badly formatted or outside of an allowable range. We should not only say “Please enter a valid date” and instead always tell the user why something is failing if we can. The Form component supports an infinite number of possible error messages per field and they are displayed simultaneously if multiple validations fail. - -### Form Alerts - -When any form field fails to validate in addition to the inline error below a field, an error message will also appear inline above the submit button indicating that some fields need to be fixed. A “fix the errors” link will scroll the user to the first input that needs attention and focus on it (putting the cursor at the end of the existing value). By default, on form submit and when tapping the “fix the errors” link we should scroll the user to the first field that needs their attention. - -![form-alert](https://user-images.githubusercontent.com/22219519/156267105-861fbe81-32cc-479d-8eff-3760bd0585b1.png) - -### Handling Server Errors - -Server errors related to form submission should appear in the Form Alert above the submit button. They should not appear in growls or other kinds of alerts. Additionally, as best practice moving forward server errors should never solely do the work that frontend validation can also do. This means that any error that can be validated in the frontend should be validated in the frontend and backend. - -Note: This is not meant to suggest that we should avoid validating in the backend if the frontend already validates. - -Note: There are edge cases where some server errors will inevitably relate to specific fields in a form with other fields unrelated to that error. We had trouble coming to a consensus on exactly how this edge case should be handled (e.g. show inline error, clear on blur, etc). For now, we will show the server error in the form alert and not inline (so the “fix the errors” link will not be present). In those cases, we will still attempt to inform the user which field needs attention, but not highlight the input or display an error below the input. We will be on the lookout for our first validation in the server that could benefit from being tied to a specific field and try to come up with a unified solution for all errors. - -## Form Submission -### Submit Button Disabling - -Submit buttons shall not be disabled or blocked from being pressed in most cases. We will allow the user to submit a form and point them in the right direction if anything needs their attention. - -The only time we won’t allow a user to press the submit button is when we have submitted the form and are waiting for a response (e.g. from the API). In this case we will show a loading indicator and additional taps on the submit button will have no effect. This is handled by the Form component and will also ensure that a form cannot be submitted multiple times. - -## Using Form.js - -The example below shows how to use [Form.js](https://github.com/Expensify/App/blob/c5a84e5b4c0b8536eed2214298a565e5237a27ca/src/components/Form.js) in our app. You can also refer to [Form.stories.js](https://github.com/Expensify/App/blob/c5a84e5b4c0b8536eed2214298a565e5237a27ca/src/stories/Form.stories.js) for more examples. - -```jsx -function validate(values) { - const errors = {}; - if (!values.routingNumber) { - errors.routingNumber = 'Please enter a routing number'; - } - if (!values.accountNumber) { - errors.accountNumber = 'Please enter an account number'; - } - return errors; -} - -function onSubmit(values) { - setTimeout(() => { - alert(`Form submitted!`); - FormActions.setIsSubmitting('TestForm', false); - }, 1000); -} - -
- // Wrapping TextInput in a View to show that Form inputs can be nested in other components - - - - - -``` - -### Props provided to Form inputs - -The following prop is available to form inputs: - -- inputID: An unique identifier for the input. - -Form.js will automatically provide the following props to any input with the inputID prop. - -- ref: A React ref that must be attached to the input. -- defaultValue: The input default value. -- errorText: The translated error text that is returned by validate for that specific input. -- onBlur: An onBlur handler that calls validate. -- onInputChange: An onChange handler that saves draft values and calls validate for that input (inputA). Passing an inputID as a second param allows inputA to manipulate the input value of the provided inputID (inputB). +This file has been moved [here](./docs/FORMS.md). diff --git a/docs/FORMS.md b/docs/FORMS.md new file mode 100644 index 000000000000..c386cb9bec1c --- /dev/null +++ b/docs/FORMS.md @@ -0,0 +1,213 @@ +# Creating and Using Forms + +This document lists specific guidelines for using our Form component and general forms guidelines. + +## General Form UI/UX + +### Labels, Placeholders, & Hints + +Labels are required for each input and should clearly mark the field. Optional text may appear below a field when a hint, suggestion, or context feels necessary. If validation fails on such a field, its error should clearly explain why without relying on the hint. Inline errors should always replace the microcopy hints. Placeholders should not be used as it’s customary for labels to appear inside form fields and animate them above the field when focused. + +![hint](https://user-images.githubusercontent.com/22219519/156266779-72deaf42-832c-453c-a5c2-1b2073b8b3b7.png) + +Labels and hints are enabled by passing the appropriate props to each input: + +```jsx + +``` + +### Character Limits + +If a field has a character limit we should give that field a max limit. This is done by passing the maxLength prop to TextInput. + +```jsx + +``` +Note: We shouldn't place a max limit on a field if the entered value can be formatted. eg: Phone number. +The phone number can be formatted in different ways. + +- 2109400803 +- +12109400803 +- (210)-940-0803 + +### Native Keyboards + +We should always set people up for success on native platforms by enabling the best keyboard for the type of input we’re asking them to provide. See [keyboardType](https://reactnative.dev/docs/0.64/textinput#keyboardtype) in the React Native documentation. + +We have a couple of keyboard types [defined](https://github.com/Expensify/App/blob/572caa9e7cf32a2d64fe0e93d171bb05a1dfb217/src/CONST.js#L357-L360) and should be used like so: + +```jsx + +``` + +### Autofill Behavior + +Forms should autofill information whenever possible i.e. they should work with browsers and password managers auto complete features. + +As a best practice we should avoid asking for information we can get via other means e.g. asking for City, State, and Zip if we can use Google Places to gather information with the least amount of hassle to the user. + +Browsers use the name prop to autofill information into the input. Here's a [reference](https://developers.google.com/web/fundamentals/design-and-ux/input/forms#recommended_input_name_and_autocomplete_attribute_values) for available values for the name prop. + +```jsx + +``` + +### Focus and Tab Behavior + +All forms should define an order in which the inputs should be filled out, and using tab / shift + tab to navigate through the form should traverse the inputs in that order/reversed order, respectively. In most cases this can be achieved by composition, i.e. rendering the components in the correct order. If we come across a situation where composition is not enough, we can: + +1. Create a local tab index state +2. Assign a tab index to each form input +3. Add an event listener to the page/component we are creating and update the tab index state on tab/shift + tab key press +4. Set focus to the input with that tab index. + +Additionally, pressing the enter key on any focused field should submit the form. + +Note: This doesn't apply to the multiline fields. To keep the browser behavior consistent, pressing enter on the multiline should not be intercepted. It should follow the default browser behavior (such as adding a newline). + +### Modifying User Input on Change + +User input that may include optional characters (e.g. (, ), - in a phone number) should never be restricted on input, nor be modified or formatted on blur. This type of input jacking is disconcerting and makes things feel broken. + +Instead we will format and clean the user input internally before using the value (e.g. making an API request where the user will never see this transformation happen). Additionally, users should always be able to copy/paste whatever characters they want into fields. + +To give a slightly more detailed example of how this would work with phone numbers we should: + +1. Allow any character to be entered in the field. +2. On blur, strip all non-number characters (with the exception of + if the API accepts it) and validate the result against the E.164 regex pattern we use for a valid phone. This change is internal and the user should not see any changes. This should be done in the validate callback passed as a prop to Form. +3. On submit, repeat validation and submit with the clean value. + +### Form Drafts + +Form inputs will NOT store draft values by default. This is to avoid accidentely storing any sensitive information like passwords, SSN or bank account information. We need to explicitly tell each form input to save draft values by passing the shouldSaveDraft prop to the input. Saving draft values is highly desireable and we should always try to save draft values. This way when a user continues a given flow they can easily pick up right where they left off if they accidentally exited a flow. Inputs with saved draft values [will be cleared when a user logs out](https://github.com/Expensify/App/blob/aa1f0f34eeba5d761657168255a1ae9aebdbd95e/src/libs/actions/SignInRedirect.js#L52) (like most data). Additionally we should clear draft data once the form is successfully submitted by calling `Onyx.set(ONYXKEY.FORM_ID, null)` in the onSubmit callback passed to Form. + +```jsx + +``` + +## Form Validation and Error handling + +### Validate on Blur and Submit + +Each individual form field that requires validation will have its own validate test defined. When the form field loses focus (blur) we will run that validate test and show feedback. A blur on one field will not cause other fields to validate or show errors unless they have already been blurred. + +All form fields will additionally be validated when the form is submitted. Although we are validating on blur this additional step is necessary to cover edge cases where forms are auto-filled or when a form is submitted by pressing enter (i.e. there will be only a ‘submit’ event and no ‘blur’ event to hook into). + +The Form component takes care of validation internally and the only requirement is that we pass a validate callback prop. The validate callback takes in the input values as argument and should return an object with shape `{[inputID]: errorMessage}`. Here's an example for a form that has two inputs, `routingNumber` and `accountNumber`: + +```js +function validate(values) { + const errors = {}; + if (!values.routingNumber) { + errors.routingNumber = props.translate(CONST.ERRORS.ROUTING_NUMBER); + } + if (!values.accountNumber) { + errors.accountNumber = props.translate(CONST.ERRORS.ACCOUNT_NUMBER); + } + return errors; +} +``` + +For a working example, check [Form story](https://github.com/Expensify/App/blob/aa1f0f34eeba5d761657168255a1ae9aebdbd95e/src/stories/Form.stories.js#L63-L72) + +### Highlight Fields and Inline Errors + +Individual form fields should be highlighted with a red error outline and present supporting inline error text below the field. Error text will be required for all required fields and optional fields that require validation. This will keep our error handling consistent and ensure we put in a good effort to help the user fix the problem by providing more information than less. + +![error](https://user-images.githubusercontent.com/22219519/156267035-af40fe93-da27-4e16-bc55-b7cd40b0f1f2.png) + +### Multiple Types of Errors for Individual Fields + +Individual fields should support multiple messages depending on validation e.g. a date could be badly formatted or outside of an allowable range. We should not only say “Please enter a valid date” and instead always tell the user why something is failing if we can. The Form component supports an infinite number of possible error messages per field and they are displayed simultaneously if multiple validations fail. + +### Form Alerts + +When any form field fails to validate in addition to the inline error below a field, an error message will also appear inline above the submit button indicating that some fields need to be fixed. A “fix the errors” link will scroll the user to the first input that needs attention and focus on it (putting the cursor at the end of the existing value). By default, on form submit and when tapping the “fix the errors” link we should scroll the user to the first field that needs their attention. + +![form-alert](https://user-images.githubusercontent.com/22219519/156267105-861fbe81-32cc-479d-8eff-3760bd0585b1.png) + +### Handling Server Errors + +Server errors related to form submission should appear in the Form Alert above the submit button. They should not appear in growls or other kinds of alerts. Additionally, as best practice moving forward server errors should never solely do the work that frontend validation can also do. This means that any error that can be validated in the frontend should be validated in the frontend and backend. + +Note: This is not meant to suggest that we should avoid validating in the backend if the frontend already validates. + +Note: There are edge cases where some server errors will inevitably relate to specific fields in a form with other fields unrelated to that error. We had trouble coming to a consensus on exactly how this edge case should be handled (e.g. show inline error, clear on blur, etc). For now, we will show the server error in the form alert and not inline (so the “fix the errors” link will not be present). In those cases, we will still attempt to inform the user which field needs attention, but not highlight the input or display an error below the input. We will be on the lookout for our first validation in the server that could benefit from being tied to a specific field and try to come up with a unified solution for all errors. + +## Form Submission +### Submit Button Disabling + +Submit buttons shall not be disabled or blocked from being pressed in most cases. We will allow the user to submit a form and point them in the right direction if anything needs their attention. + +The only time we won’t allow a user to press the submit button is when we have submitted the form and are waiting for a response (e.g. from the API). In this case we will show a loading indicator and additional taps on the submit button will have no effect. This is handled by the Form component and will also ensure that a form cannot be submitted multiple times. + +## Using Form.js + +The example below shows how to use [Form.js](https://github.com/Expensify/App/blob/c5a84e5b4c0b8536eed2214298a565e5237a27ca/src/components/Form.js) in our app. You can also refer to [Form.stories.js](https://github.com/Expensify/App/blob/c5a84e5b4c0b8536eed2214298a565e5237a27ca/src/stories/Form.stories.js) for more examples. + +```jsx +function validate(values) { + const errors = {}; + if (!values.routingNumber) { + errors.routingNumber = 'Please enter a routing number'; + } + if (!values.accountNumber) { + errors.accountNumber = 'Please enter an account number'; + } + return errors; +} + +function onSubmit(values) { + setTimeout(() => { + alert(`Form submitted!`); + FormActions.setIsSubmitting('TestForm', false); + }, 1000); +} + +
+ // Wrapping TextInput in a View to show that Form inputs can be nested in other components + + + + + +``` + +### Props provided to Form inputs + +The following prop is available to form inputs: + +- inputID: An unique identifier for the input. + +Form.js will automatically provide the following props to any input with the inputID prop. + +- ref: A React ref that must be attached to the input. +- defaultValue: The input default value. +- errorText: The translated error text that is returned by validate for that specific input. +- onBlur: An onBlur handler that calls validate. +- onInputChange: An onChange handler that saves draft values and calls validate for that input (inputA). Passing an inputID as a second param allows inputA to manipulate the input value of the provided inputID (inputB). From 28a7cb25b0ff384cdc7a4a911272dd491ffe6821 Mon Sep 17 00:00:00 2001 From: Tim Golen Date: Mon, 4 Jul 2022 13:42:37 -0600 Subject: [PATCH 02/11] Move how to create a plan document --- CONTRIBUTING.md | 2 +- HOW_TO_CREATE_A_PLAN.md | 46 +----------------------------------- docs/HOW_TO_CREATE_A_PLAN.md | 45 +++++++++++++++++++++++++++++++++++ 3 files changed, 47 insertions(+), 46 deletions(-) create mode 100644 docs/HOW_TO_CREATE_A_PLAN.md diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index fee06cf7c896..f934310ee773 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -55,7 +55,7 @@ Please follow these steps to propose a job: 2. If your bug or enhancement matches an existing issue, please feel free to comment on that GitHub issue with your findings if you think it will help solve the issue. 4. If there is no existing GitHub issue or Upwork job, check if the issue is happening on prod (as opposed to only happening on dev) 5. If the issue is just in dev then it means it's a new issue and has not been deployed to production. In this case, you should try to find the offending PR and comment in the issue tied to the PR and ask the assigned users to add the `DeployBlockerCash` label. If you can't find it, follow the reporting instructions in the next item, but note that the issue is a regression only found in dev and not in prod. -6. If the issue happens in main, staging or production then report the issue(s) in the [#expensify-open-source](https://github.com/Expensify/App/blob/main/CONTRIBUTING.md#asking-questions) Slack channel, prefixed with `BUG:` or `Feature Request:`. Please use the templates for bugs and feature requests that are bookmarked in #expensify-open-source. View [this guide](https://github.com/Expensify/App/blob/main/HOW_TO_CREATE_A_PLAN.md) for help creating a plan when proposing a feature request. +6. If the issue happens in main, staging or production then report the issue(s) in the [#expensify-open-source](https://github.com/Expensify/App/blob/main/CONTRIBUTING.md#asking-questions) Slack channel, prefixed with `BUG:` or `Feature Request:`. Please use the templates for bugs and feature requests that are bookmarked in #expensify-open-source. View [this guide](https://github.com/Expensify/App/blob/main/docs/HOW_TO_CREATE_A_PLAN.md) for help creating a plan when proposing a feature request. 7. After review in #expensify-open-source, if you've provided a quality proposal that we choose to implement, a GitHub issue will be created and your Slack handle will be included in the original post after `Issue reported by:` 8. If an external contributor other than yourself is hired to work on the issue, you will also be hired for the same job in Upwork. No additional work is needed. If the issue is fixed internally, a dedicated job will be created to hire and pay you after the issue is fixed. 9. Payment will be made 7 days after code is deployed to production if there are no regressions. If a regression is discovered, payment will be issued 7 days after all regressions are fixed. diff --git a/HOW_TO_CREATE_A_PLAN.md b/HOW_TO_CREATE_A_PLAN.md index cb1f8b4a7f40..ae0616ed6157 100644 --- a/HOW_TO_CREATE_A_PLAN.md +++ b/HOW_TO_CREATE_A_PLAN.md @@ -1,45 +1 @@ -## How to create a plan - Contributor version - -### Preamble -If you’re new to working with Expensify, it’s important to know the philosophy behind the design of Expensify’s products and the processes that wrap around them. -- Problems are identified and defined before solutions are proposed -- Features are not added to satisfy personal preferences -- Features are added thoughtfully and minimally - -It’s also important to be familiar with our existing documentation including [CONTRIBUTING.md](https://github.com/Expensify/App/blob/main/CONTRIBUTING.md) and [README.md](https://github.com/Expensify/App/blob/main/README.md) - -## Step 1: Define the problem -What is the problem that needs solving? -- Be broad -- Define who experiences the problem -- Does this affect the customer experience? How so? -- How many customers would be affected by this? -- Define why it is a problem -- Resist the temptation to reverse-engineer a Solution into a matching Problem. (ie. Solution: Let's get a car! Problem: We don’t have a car.) -- There may be more than one problem, so define all of them -- Keep the language simple - Reduce every word down to its most plain and straightforward manner. Avoid jargon. If you use abbreviations, include the full spelling the first time. -- Tear apart your statements and have others tear them apart for you until everybody agrees unambiguously as to the nature of the problem. If needed, continue the discussion in #expensify-open-source. - -Identify which problems are actually independent and can be solved by themselves, versus which are actually a series of dependent but otherwise separate problems that can be solved one at a time. - -The broader and better contextualized the problem definition, the greater opportunity for discovering radical, outside-of-the-box solutions. The odds of achieving something are dramatically improved by clearly defining what it is. Take at least as much care perfecting your problem definition as you do the solution, because the perfect solution to the wrong problem is typically an awful solution to the right one. - -## Step 2: Workshop the solutions -Draft as many solutions as you can for each problem, including any existing workarounds - “Do nothing” is always a potential solution. Be as creative or audacious as you want when workshopping potential solutions. - -Once you have your solutions, it’s time to decide on the preferred solution (alternate solutions may be included at a later stage). Aspects that may promote one solution to “preferred” status: -- ROI (return on investment) - the current value of the problem, versus the cost of your solution -- Work involved - is one solution just as effective but would take half the time to create? -- Adding or moving “things” - does one solution have less consumer impact? -- Future-proofing - does one solution have more longevity or pave the way for future development? -- Independence - does a solution rely on a different problem to be solved first? Does it rely on another piece to be done later? - -If you are finding the solution to be difficult, go back and beat harder on the problem to break it up into smaller pieces. Keep repeating until you have a general list of prioritized stages, with early stages solving the dependencies required by later stages, each of which is extremely well defined, with a reasonably obvious preferred solution. - -## Step 3: Write out each problem and solution (P/S statement) -Have a trusted peer or two proof your P/S statement and help you ensure it is well-defined. If you're in need of a peer to proof, post in #expensify-open-source to ask for help. Refine it and then share with another peer or two until you have a clear, understandable P/S statement. The more complex the problem and solution, the more people should review it. Keep going back to step 1 if needed. - -## Step 4: Propose it -Once you have a well-defined problem and solution, post your problem and solution as separate comments in ​​#expensify-open-source in Slack and be prepared to answer questions and defend your choices. Also, be prepared to hear better solutions that may completely change your P/S statement. - -Depending on the complexity of the P/S Statement, an internal Design Doc may be required (Expensify employees will take it from there). +This file has been moved [here](./docs/HOW_TO_CREATE_A_PLAN.md). diff --git a/docs/HOW_TO_CREATE_A_PLAN.md b/docs/HOW_TO_CREATE_A_PLAN.md new file mode 100644 index 000000000000..cb1f8b4a7f40 --- /dev/null +++ b/docs/HOW_TO_CREATE_A_PLAN.md @@ -0,0 +1,45 @@ +## How to create a plan - Contributor version + +### Preamble +If you’re new to working with Expensify, it’s important to know the philosophy behind the design of Expensify’s products and the processes that wrap around them. +- Problems are identified and defined before solutions are proposed +- Features are not added to satisfy personal preferences +- Features are added thoughtfully and minimally + +It’s also important to be familiar with our existing documentation including [CONTRIBUTING.md](https://github.com/Expensify/App/blob/main/CONTRIBUTING.md) and [README.md](https://github.com/Expensify/App/blob/main/README.md) + +## Step 1: Define the problem +What is the problem that needs solving? +- Be broad +- Define who experiences the problem +- Does this affect the customer experience? How so? +- How many customers would be affected by this? +- Define why it is a problem +- Resist the temptation to reverse-engineer a Solution into a matching Problem. (ie. Solution: Let's get a car! Problem: We don’t have a car.) +- There may be more than one problem, so define all of them +- Keep the language simple - Reduce every word down to its most plain and straightforward manner. Avoid jargon. If you use abbreviations, include the full spelling the first time. +- Tear apart your statements and have others tear them apart for you until everybody agrees unambiguously as to the nature of the problem. If needed, continue the discussion in #expensify-open-source. + +Identify which problems are actually independent and can be solved by themselves, versus which are actually a series of dependent but otherwise separate problems that can be solved one at a time. + +The broader and better contextualized the problem definition, the greater opportunity for discovering radical, outside-of-the-box solutions. The odds of achieving something are dramatically improved by clearly defining what it is. Take at least as much care perfecting your problem definition as you do the solution, because the perfect solution to the wrong problem is typically an awful solution to the right one. + +## Step 2: Workshop the solutions +Draft as many solutions as you can for each problem, including any existing workarounds - “Do nothing” is always a potential solution. Be as creative or audacious as you want when workshopping potential solutions. + +Once you have your solutions, it’s time to decide on the preferred solution (alternate solutions may be included at a later stage). Aspects that may promote one solution to “preferred” status: +- ROI (return on investment) - the current value of the problem, versus the cost of your solution +- Work involved - is one solution just as effective but would take half the time to create? +- Adding or moving “things” - does one solution have less consumer impact? +- Future-proofing - does one solution have more longevity or pave the way for future development? +- Independence - does a solution rely on a different problem to be solved first? Does it rely on another piece to be done later? + +If you are finding the solution to be difficult, go back and beat harder on the problem to break it up into smaller pieces. Keep repeating until you have a general list of prioritized stages, with early stages solving the dependencies required by later stages, each of which is extremely well defined, with a reasonably obvious preferred solution. + +## Step 3: Write out each problem and solution (P/S statement) +Have a trusted peer or two proof your P/S statement and help you ensure it is well-defined. If you're in need of a peer to proof, post in #expensify-open-source to ask for help. Refine it and then share with another peer or two until you have a clear, understandable P/S statement. The more complex the problem and solution, the more people should review it. Keep going back to step 1 if needed. + +## Step 4: Propose it +Once you have a well-defined problem and solution, post your problem and solution as separate comments in ​​#expensify-open-source in Slack and be prepared to answer questions and defend your choices. Also, be prepared to hear better solutions that may completely change your P/S statement. + +Depending on the complexity of the P/S Statement, an internal Design Doc may be required (Expensify employees will take it from there). From 748187c57078471520c911434c23cb57823a0b61 Mon Sep 17 00:00:00 2001 From: Tim Golen Date: Mon, 4 Jul 2022 13:46:45 -0600 Subject: [PATCH 03/11] Move performance document --- OfflineUX.md | 134 +----------------- PERFORMANCE.md | 95 +------------ README.md | 8 +- docs/OFFLINE_UX.md | 133 +++++++++++++++++ .../OfflineUX_Patterns_Flowchart.png | Bin docs/PERFORMANCE.md | 94 ++++++++++++ {web => docs}/data_flow.png | Bin 7 files changed, 233 insertions(+), 231 deletions(-) create mode 100644 docs/OFFLINE_UX.md rename {web => docs}/OfflineUX_Patterns_Flowchart.png (100%) create mode 100644 docs/PERFORMANCE.md rename {web => docs}/data_flow.png (100%) diff --git a/OfflineUX.md b/OfflineUX.md index 801523a6aa63..a11c1d72f0bc 100644 --- a/OfflineUX.md +++ b/OfflineUX.md @@ -1,133 +1 @@ -#### Offline UX Patterns - -### Contents -* [Motivation & Philosophy](#motivation-&-philosophy) -* [UX Pattern Flowchart](#ux-pattern-flowchart) -* [Answering Questions on the Flow Chart](#answering-questions-on-the-flowchart) -* [Description of the Patterns](#description-of-the-patterns) - - [None - No Offline Behavior](#none---no-offline-behavior) - - [A - Optimistic Without Feedback](#a---optimistic-without-feedback) - - [B - Optimistic With Feedback](#b---optimistic-with-feedback) - - [C - Blocking Form](#c---blocking-form) - - [D - Full Page Blocking](#d---full-page-blocking) - -### Motivation & Philosophy - -Understanding the offline behavior of our app is vital to becoming a productive contributor to the Expensify codebase. Our mission is to support our users in every possible environment, and often our app is used in places where a stable internet connection is not guaranteed. - -The most important concept to keep in mind while reading this document is that we want to allow users to do as much as possible when offline. At first, this might seem impossible because almost everything the user can touch in our app is related to an API request. However, in many cases, we can save that API request and assume it will succeed when the user is back online. We then allow the user to proceed as if their request already succeeded. We call this an optimistic response. Here, we use the word **optimistic** to indicate that we're confident the request will succeed when the user is online, and we know what that successful response will look like. - -
-Example: Pinning a chat - -When a user clicks the pin button on a chat, two things should happen. - -1. **API Request:** We send a request to the API to ensure the change is saved in the database. This way the chat is pinned on all the user's devices, and will remain pinned even if they leave the app and come back. - -2. **UI Changes:** The chat should go to the top of the list with the other pinned chats, and the pin button should look darker than it did before. This is visual feedback that clicking the pin button worked. - -If the user is offline, we don't need to wait for the API request to finish before doing all that visual stuff because this particular API request has almost no way of failing, and we know what the server will return in advance. That means we can safely assume that when we retry the command, it will succeed and that's why we let the user continue using the app as if the action succeeded. - -
- -The example we just looked at is nice and simple, but some actions should not use this approach (example: requesting money from another user). For these types of actions, we can't simply proceed as if the request already finished. Here are some reasons why: - -1. We don't know _how_ to proceed because of a lack of information (often the server returns data that we wouldn't be able to guess the content of). - -2. We may be able to guess what a successful request would look like, but we don't want to misguide the user into believing their action was completed. For example, we don't want the user to believe that a financial transaction has been made when it actually hasn't. - -To handle problems like this, we have developed offline UX patterns and guidance on when to use them. Every feature of this application should fit into one of these patterns. - -### Descriptions of the UX Patterns - -# None - No Offline Behavior - -There’s no specific UI for this case. The feature either looks totally normal and works as expected (because it doesn’t need the server to function) or the feature looks like it did whenever connection was lost. - -**Used when…** - - there is no interaction with the server in any way - - or data is READ from the server and does not need to show up-to-date data. The user will see stale data until the new data is put into Onyx and then the view updates to show the new data. - - **How to implement:** Use [`API.read()`](https://github.com/Expensify/App/blob/3493f3ca3a1dc6cdbf9cb8bd342866fcaf45cf1d/src/libs/API.js#L53-L55). - -# A - Optimistic Without Feedback Pattern - -This is the pattern where we queue the request to be sent when the user is online and we continue as if the request succeeded. - -**Used when…** - - the user should be given instant feedback and - - the user does not need to know when the change is done on the server in the background - -**How to implement:** Use [`API.write()`](https://github.com/Expensify/App/blob/3493f3ca3a1dc6cdbf9cb8bd342866fcaf45cf1d/src/libs/API.js#L7-L28) to implement this pattern. For this pattern we should only put `optimisticData` in the options. We don't need successData or failData as we don't care what response comes back at all. - -# B - Optimistic WITH Feedback Pattern -This pattern queues the API request, but also makes sure that the user is aware that the request hasn’t been sent yet **when the user is offline**. When the user is online, the feature should just look like it succeeds immediately (we dont want the offline UI to flicker on and off when the user is online). - -**Used when…** - - the user needs feedback that data will be sent to the server later -This is a minority use case at the moment, but INCREDIBLY HELPFUL for the user, so proceed with cautious optimism. - -**How to implement:** Use API.write() to implement this pattern. Optimistic data should include some pending state for the action that is reflected in the UI. Success/failure data should revert the pending state and/or set a failure state accordingly. - -# C - Blocking Form UI Pattern -This pattern greys out the submit button on a form and does not allow the form to be submitted. We also show a "You appear offline" message near the bottom of the screen. Importantly, we _do_ let the user fill out the form fields. That data gets saved locally so they don’t have to fill it out again once online. - -**Used when…** - - a form is used to make a WRITE request to the server and - - server has to do some validation of the parameters that can’t be done in the client or - - server response will be unknown so it cannot be done optimistically - - If the request is moving money - -**How to implement:** Use the `` component. This pattern should use the `API.write()` method. - -# D - Full Page Blocking UI Pattern -This pattern blocks the user from interacting with an entire page. - -**Used when…** - - blocking READ is being performed. This occurs when the data that a user sees cannot be stale data and the data can only be displayed after fetching it from the server (eg. Plaid's list of bank accounts) - - the app is offline and the data cannot be fetched - - an error occurs when fetching the data and the user needs instructions on what to do next -This should only be used in the most extreme cases when all other options have been completely and utterly exhausted - -**How to implement:** Wrap the component you're working on in a `` component. - -### UX Pattern Flow Chart - -The following flowchart can be used to determine which UX pattern should be used. - -![New Expensify Data Flow Chart](/web/OfflineUX_Patterns_Flowchart.png) - -### Answering Questions on the Flow Chart - -The numbers in this section correlate to the numbers in each decision box above (the diamond shapes). - -1. Does the feature interact with the server? - -If you're changing an existing feature, you can open the network tab of dev tools to see if any network requests are being made when you use the feature. If network requests are being made, the answer to this question is YES. Note: Sometimes you may see requests that happen to fire at the same time as the feature you're working on, so be sure to double check. -If you're making a new feature, think about whether any data would need to be retrieved or stored from anywhere other than the local device. If data needs to be stored to or retrieved from the server, then the answer is YES. - -2. What type of request is being made? - -If there's new data being saved on the server, you're making a WRITE request. If you're retrieving existing data from the server, you're making a READ request. If both things are happening, that's a WRITE request. - -3. Is it OK for the user to see stale data? - -Example: The payment method list. We don't want the user to see a payment method that we no longer support, not even while the payment methods are being loaded from the server (or while the user is offline). Therefore, we answer NO, which leads us to the blocking UI. This way the user won't see stale data while we load the payment methods. - -4. Is the UI a form? - -An easy way to tell if something is a form is to try and find a submit button. If a submit button is present or if the user is filling out form inputs, answer YES to this question. - -5. Can the server response be anticipated? - -Answer NO if there is data coming back from the server that we can't know (example: a list of bank accounts from Plaid, input validation that the server must perform). Answer YES if we can know what the response from the server would be. - -6. Is there validation done on the server that can't be done on the front end? - -If there is some validation happening on the server that needs to happen before the feature can work, then we answer YES to this question. Remember, this is referring to validation that cannot happen on the front end (e.g. reusing an existing password when resetting a password). For example, if we want to set up a bank account then our answer to this question is YES (because we can’t suggest to the user that their request succeeded when really it hasn’t been sent yet–their card wouldn’t work!) - -This question can be tricky, so if you're unsure, please ask a question in the #expensify-open-source slack room and tag @contributor-management-engineering. - -7. Does the user need to know if the action was successful? - -Think back to the pinning example from above: the user doesn’t need to know that their pinned report's NVP has been updated. To them the impact of clicking the pin button is that their chat is at the top of the LHN. It makes no difference to them if the server has been updated or not, so the answer would be NO. Now let’s consider sending a payment request to another user. In this example, the user needs to know if their request was actually sent, so our answer is YES. +This file has been moved [here](./docs/OFFLINE_UX.md). diff --git a/PERFORMANCE.md b/PERFORMANCE.md index a80b52d5885f..2356a29a8a17 100644 --- a/PERFORMANCE.md +++ b/PERFORMANCE.md @@ -1,94 +1 @@ -# React Performance Tips - -- Always test performance with the production build as development mode is not optimized. -- Use [`PureComponent`](https://reactjs.org/docs/react-api.html#reactpurecomponent), [`React.memo()`](https://reactjs.org/docs/react-api.html#reactmemo), and [`shouldComponentUpdate()`](https://reactjs.org/docs/react-component.html#shouldcomponentupdate) to prevent re-rendering expensive components. -- Using a combination of [React DevTools Profiler](https://chrome.google.com/webstore/detail/react-developer-tools/fmkadmapgofadopljbjfkapdkoienihi?hl=en) and [Chrome Dev Tools Performance Timing](https://calibreapp.com/blog/react-performance-profiling-optimization) can help identify unnecessary re-renders. Both tools can be used to time an interaction like the app starting up or navigating to a new screen. -- Watch out for [very large lists](https://reactnative.dev/docs/optimizing-flatlist-configuration) and things like `Image` components re-fetching images on render when a remote uri did not change. -- Avoid the temptation to over-optimize. There is added cost in both code complexity and performance when adding checks like `shouldComponentUpdate()`. Be selective about when you use this and make sure there is a measureable difference before proposing the change. As a very general rule it should be measurably faster to run logic to avoid the re-render (e.g. do a deep comparison) than it would be to let React take care of it without any extra intervention from us. -- Use caution when adding subscriptions that might re-render very large trees of components e.g. subscribing to state that changes often (current report, current route, etc) in the app root. -- Avoid using arrow function callbacks in components that are expensive to re-render. React will re-render this component since each time the parent renders it creates a new instance of that function. **Alternative:** Bind the method in the constructor instead. - -## Tools - -### Chrome Dev Tools > Performance > Timing (Web Only) - -- Profiling in Chrome Dev Tools performance tab in the "Timing" section -- This will show various components and how long they took to render. It can be a little intense to dig through it all at first, but the more time you spend with it the easier it gets to separate the signal from noise. -- The timing information might be inaccurate in development mode since this slows things down a ton. However, it's still useful for seeing which things take the longest and it's not too difficult to look and see which things are re-rendering. - -**Suggested:** [React Performance Profiling](https://calibreapp.com/blog/react-performance-profiling-optimization) - -### Hermes Profiling (Android only) - -It's possible, but slightly trickier to profile the JS running on Android devices as it does not run in a browser but a JS VM that React Native must spin up first then run the app code. The VM we are currently using on both Android and iOS is called [Hermes](https://reactnative.dev/docs/profile-hermes) and is developed by Facebook. - -In order to profile with Hermes follow these steps: - -- In the metro bundler window press `d` on your keyboard to bring up the developer menu on your device. -- Select "Settings" -- Select "Start Sampling Profiler on Init" -- In metro bundler refresh by pressing r -- The app will start up and a profile will begin -- Once the app loads take whatever action you want to profile -- Press `d` again and select "Disable Sampling Profiler" -- A toast should appear with a path to a profile -- We need to then convert this into something Chrome Dev Tools can use by typing into terminal `react-native profile-hermes .` -- This should create a json file in the directory where we typed the previous command that we can load up into Chrome Dev Tools "Performance" tab via the "Load Profile" option and inspect further. - -### React DevTools Profiler -- The React DevTools Profiler can also be used to detect similar information to Chrome Dev Tools, but is a little more streamlined. There is also an options cog where you can filter events by cutting at a specified millisecond (length it took for the thing to happen) -- Try checking the option to "Record why each component rendered while profiling". This may provide insights into why the component rendered unnecessarily. - -**Suggested:** [Deep Dive with the React DevTools creator](https://www.youtube.com/watch?v=nySib7ipZdk) - -### Why Did You Render? -- Why Did You Render (WDYR) sends console notifications about potentially avoidable component re-renders. -- It can also help to simply track when and why a certain component re-renders. -- To enable it, set `USE_WDYR=true` in your `.env` file. -- You can add or exclude tracked components by their `displayName` in `wdyr.js`. -- Open the browser console to see WDYR notifications. - -**Suggested** [Why Did You Render docs](https://github.com/welldone-software/why-did-you-render) - -### Performance Metrics (Opt-In on local release builds) - -To capture reliable performance metrics for native app launch we must test against a release build. To make this easier for everyone to do we created an opt-in tool (using [`react-native-performance`](https://github.com/oblador/react-native-performance) that will capture metrics and display them in an alert once the app becomes interactive. To set this up just set `CAPTURE_METRICS=true` in your `.env` file then create a release build on iOS or Android. The metrics this tool shows are as follows: - -- `nativeLaunch` - Total time for the native process to intialize -- `runJSBundle` - Total time to parse and execute the JS bundle -- `timeToInteractive` - Rough TTI (Time to Interactive). Includes native init time + sidebar UI partially loaded - -#### How to create a Release Build on Android - -- Create a keystore by running `keytool -genkey -v -keystore your_key_name.keystore -alias your_key_alias -keyalg RSA -keysize 2048 -validity 10000` -- Fill out all the prompts with any info and give it a password -- Drag the generated keystore to `/android/app` -- Hardcode the values to the gradle config like so: - -``` -signingConfigs { - release { - storeFile file('your_key_name.keystore') - storePassword 'Password1' - keyAlias 'your_key_alias' - keyPassword 'Password1' - } -``` -- Delete any existing apps off emulator or device -- Run `react-native run-android --variant release` - -## Reconciliation - -React is pretty smart and in many cases is able to tell if something needs to update. The process by which React goes about updating the "tree" or view heirarchy is called reconciliation. If React thinks something needs to update it will render it again. React also assumes that if a parent component rendered then it's child should also re-render. - -Re-rendering can be expensive at times and when dealing with nested props or state React may render when it doesn't need to which can be wasteful. A good example of this is a component that is being passed an object as a prop. Let's say the component only requires one or two properties from that object in order to build it's view, but doesn't care about some others. React will still re-render that component even if nothing it cares about has changed. Most of the time this is fine since reconciliation is pretty fast. But we might run into performance issues when re-rendering massive lists. - -In this example, the most preferable solution would be to **only pass the properties that the object needs to know about** to the component in the first place. - -Another option would be to use `shouldComponentUpdate` or `React.memo()` to add more specific rules comparing `props` to **explicitly tell React not to perform a re-render**. - -React might still take some time to re-render a component when it's parent component renders. If it takes a long time to re-render the child even though we have no props changing then we can use `PureComponent` or `React.memo()` (without a callback) which will "shallow compare" the `props` to see if a component should re-render. - -If you aren't sure what exactly is changing about some deeply nested object prop you can use `Performance.diffObject()` method in `componentDidUpdate()` which should show you exactly what is changing from one update to the next. - -**Suggested:** [React Docs - Reconciliation](https://reactjs.org/docs/reconciliation.html) +This file has been moved [here](./docs/PERFORMANCE.md). diff --git a/README.md b/README.md index 45f2cc90f46a..90ed55eeaa7e 100644 --- a/README.md +++ b/README.md @@ -20,7 +20,7 @@ #### Additional Reading * [API Details](docs/API.md) -* [Offline First](./OfflineUX.md) +* [Offline First](./docs/OFFLINE_UX.md) * [Contributing to Expensify](./CONTRIBUTING.md) * [Expensify Code of Conduct](./CODE_OF_CONDUCT.md) * [Contributor License Agreement](./CLA.md) @@ -86,7 +86,7 @@ variables referenced here get updated since your local `.env` file is ignored. requests to the backend. External contributors should set this to `true` otherwise they'll have CORS errors. If you don't want to start the proxy server set this explicitly to `false` - `CAPTURE_METRICS` (optional) - Set this to `true` to capture performance metrics and see them in Flipper - see [PERFORMANCE.md](PERFORMANCE.md#performance-metrics-opt-in-on-local-release-builds) for more information + see [PERFORMANCE.md](./docs/PERFORMANCE.md#performance-metrics-opt-in-on-local-release-builds) for more information - `ONYX_METRICS` (optional) - Set this to `true` to capture even more performance metrics and see them in Flipper see [React-Native-Onyx#benchmarks](https://github.com/Expensify/react-native-onyx#benchmarks) for more information @@ -286,9 +286,9 @@ This application is built with the following principles. 4. Brain pushes data into UI inputs (Device input -> React component). 5. UI inputs push data to the server (React component -> Action -> XHR to server). 6. Go to 1 - ![New Expensify Data Flow Chart](/web/data_flow.png) + ![New Expensify Data Flow Chart](/docs/data_flow.png) 1. **Offline first** - - Be sure to read [OfflineFirst.md](./OfflineUX.md)! + - Be sure to read [OFFLINE_UX.md](./docs/OFFLINE_UX.md)! - All data that is brought into the app and is necessary to display the app when offline should be stored on disk in persistent storage (eg. localStorage on browser platforms). [AsyncStorage](https://reactnative.dev/docs/asyncstorage) is a cross-platform abstraction layer that is used to access persistent storage. - All data that is displayed, comes from persistent storage. 1. **UI Binds to data on disk** diff --git a/docs/OFFLINE_UX.md b/docs/OFFLINE_UX.md new file mode 100644 index 000000000000..48bd6c12d894 --- /dev/null +++ b/docs/OFFLINE_UX.md @@ -0,0 +1,133 @@ +#### Offline UX Patterns + +### Contents +* [Motivation & Philosophy](#motivation-&-philosophy) +* [UX Pattern Flowchart](#ux-pattern-flowchart) +* [Answering Questions on the Flow Chart](#answering-questions-on-the-flowchart) +* [Description of the Patterns](#description-of-the-patterns) + - [None - No Offline Behavior](#none---no-offline-behavior) + - [A - Optimistic Without Feedback](#a---optimistic-without-feedback) + - [B - Optimistic With Feedback](#b---optimistic-with-feedback) + - [C - Blocking Form](#c---blocking-form) + - [D - Full Page Blocking](#d---full-page-blocking) + +### Motivation & Philosophy + +Understanding the offline behavior of our app is vital to becoming a productive contributor to the Expensify codebase. Our mission is to support our users in every possible environment, and often our app is used in places where a stable internet connection is not guaranteed. + +The most important concept to keep in mind while reading this document is that we want to allow users to do as much as possible when offline. At first, this might seem impossible because almost everything the user can touch in our app is related to an API request. However, in many cases, we can save that API request and assume it will succeed when the user is back online. We then allow the user to proceed as if their request already succeeded. We call this an optimistic response. Here, we use the word **optimistic** to indicate that we're confident the request will succeed when the user is online, and we know what that successful response will look like. + +
+Example: Pinning a chat + +When a user clicks the pin button on a chat, two things should happen. + +1. **API Request:** We send a request to the API to ensure the change is saved in the database. This way the chat is pinned on all the user's devices, and will remain pinned even if they leave the app and come back. + +2. **UI Changes:** The chat should go to the top of the list with the other pinned chats, and the pin button should look darker than it did before. This is visual feedback that clicking the pin button worked. + +If the user is offline, we don't need to wait for the API request to finish before doing all that visual stuff because this particular API request has almost no way of failing, and we know what the server will return in advance. That means we can safely assume that when we retry the command, it will succeed and that's why we let the user continue using the app as if the action succeeded. + +
+ +The example we just looked at is nice and simple, but some actions should not use this approach (example: requesting money from another user). For these types of actions, we can't simply proceed as if the request already finished. Here are some reasons why: + +1. We don't know _how_ to proceed because of a lack of information (often the server returns data that we wouldn't be able to guess the content of). + +2. We may be able to guess what a successful request would look like, but we don't want to misguide the user into believing their action was completed. For example, we don't want the user to believe that a financial transaction has been made when it actually hasn't. + +To handle problems like this, we have developed offline UX patterns and guidance on when to use them. Every feature of this application should fit into one of these patterns. + +### Descriptions of the UX Patterns + +# None - No Offline Behavior + +There’s no specific UI for this case. The feature either looks totally normal and works as expected (because it doesn’t need the server to function) or the feature looks like it did whenever connection was lost. + +**Used when…** +- there is no interaction with the server in any way +- or data is READ from the server and does not need to show up-to-date data. The user will see stale data until the new data is put into Onyx and then the view updates to show the new data. + +**How to implement:** Use [`API.read()`](https://github.com/Expensify/App/blob/3493f3ca3a1dc6cdbf9cb8bd342866fcaf45cf1d/src/libs/API.js#L53-L55). + +# A - Optimistic Without Feedback Pattern + +This is the pattern where we queue the request to be sent when the user is online and we continue as if the request succeeded. + +**Used when…** +- the user should be given instant feedback and +- the user does not need to know when the change is done on the server in the background + +**How to implement:** Use [`API.write()`](https://github.com/Expensify/App/blob/3493f3ca3a1dc6cdbf9cb8bd342866fcaf45cf1d/src/libs/API.js#L7-L28) to implement this pattern. For this pattern we should only put `optimisticData` in the options. We don't need successData or failData as we don't care what response comes back at all. + +# B - Optimistic WITH Feedback Pattern +This pattern queues the API request, but also makes sure that the user is aware that the request hasn’t been sent yet **when the user is offline**. When the user is online, the feature should just look like it succeeds immediately (we dont want the offline UI to flicker on and off when the user is online). + +**Used when…** +- the user needs feedback that data will be sent to the server later + This is a minority use case at the moment, but INCREDIBLY HELPFUL for the user, so proceed with cautious optimism. + +**How to implement:** Use API.write() to implement this pattern. Optimistic data should include some pending state for the action that is reflected in the UI. Success/failure data should revert the pending state and/or set a failure state accordingly. + +# C - Blocking Form UI Pattern +This pattern greys out the submit button on a form and does not allow the form to be submitted. We also show a "You appear offline" message near the bottom of the screen. Importantly, we _do_ let the user fill out the form fields. That data gets saved locally so they don’t have to fill it out again once online. + +**Used when…** +- a form is used to make a WRITE request to the server and +- server has to do some validation of the parameters that can’t be done in the client or +- server response will be unknown so it cannot be done optimistically +- If the request is moving money + +**How to implement:** Use the `` component. This pattern should use the `API.write()` method. + +# D - Full Page Blocking UI Pattern +This pattern blocks the user from interacting with an entire page. + +**Used when…** +- blocking READ is being performed. This occurs when the data that a user sees cannot be stale data and the data can only be displayed after fetching it from the server (eg. Plaid's list of bank accounts) +- the app is offline and the data cannot be fetched +- an error occurs when fetching the data and the user needs instructions on what to do next + This should only be used in the most extreme cases when all other options have been completely and utterly exhausted + +**How to implement:** Wrap the component you're working on in a `` component. + +### UX Pattern Flow Chart + +The following flowchart can be used to determine which UX pattern should be used. + +![New Expensify Data Flow Chart](/docs/OfflineUX_Patterns_Flowchart.png) + +### Answering Questions on the Flow Chart + +The numbers in this section correlate to the numbers in each decision box above (the diamond shapes). + +1. Does the feature interact with the server? + +If you're changing an existing feature, you can open the network tab of dev tools to see if any network requests are being made when you use the feature. If network requests are being made, the answer to this question is YES. Note: Sometimes you may see requests that happen to fire at the same time as the feature you're working on, so be sure to double check. +If you're making a new feature, think about whether any data would need to be retrieved or stored from anywhere other than the local device. If data needs to be stored to or retrieved from the server, then the answer is YES. + +2. What type of request is being made? + +If there's new data being saved on the server, you're making a WRITE request. If you're retrieving existing data from the server, you're making a READ request. If both things are happening, that's a WRITE request. + +3. Is it OK for the user to see stale data? + +Example: The payment method list. We don't want the user to see a payment method that we no longer support, not even while the payment methods are being loaded from the server (or while the user is offline). Therefore, we answer NO, which leads us to the blocking UI. This way the user won't see stale data while we load the payment methods. + +4. Is the UI a form? + +An easy way to tell if something is a form is to try and find a submit button. If a submit button is present or if the user is filling out form inputs, answer YES to this question. + +5. Can the server response be anticipated? + +Answer NO if there is data coming back from the server that we can't know (example: a list of bank accounts from Plaid, input validation that the server must perform). Answer YES if we can know what the response from the server would be. + +6. Is there validation done on the server that can't be done on the front end? + +If there is some validation happening on the server that needs to happen before the feature can work, then we answer YES to this question. Remember, this is referring to validation that cannot happen on the front end (e.g. reusing an existing password when resetting a password). For example, if we want to set up a bank account then our answer to this question is YES (because we can’t suggest to the user that their request succeeded when really it hasn’t been sent yet–their card wouldn’t work!) + +This question can be tricky, so if you're unsure, please ask a question in the #expensify-open-source slack room and tag @contributor-management-engineering. + +7. Does the user need to know if the action was successful? + +Think back to the pinning example from above: the user doesn’t need to know that their pinned report's NVP has been updated. To them the impact of clicking the pin button is that their chat is at the top of the LHN. It makes no difference to them if the server has been updated or not, so the answer would be NO. Now let’s consider sending a payment request to another user. In this example, the user needs to know if their request was actually sent, so our answer is YES. diff --git a/web/OfflineUX_Patterns_Flowchart.png b/docs/OfflineUX_Patterns_Flowchart.png similarity index 100% rename from web/OfflineUX_Patterns_Flowchart.png rename to docs/OfflineUX_Patterns_Flowchart.png diff --git a/docs/PERFORMANCE.md b/docs/PERFORMANCE.md new file mode 100644 index 000000000000..a80b52d5885f --- /dev/null +++ b/docs/PERFORMANCE.md @@ -0,0 +1,94 @@ +# React Performance Tips + +- Always test performance with the production build as development mode is not optimized. +- Use [`PureComponent`](https://reactjs.org/docs/react-api.html#reactpurecomponent), [`React.memo()`](https://reactjs.org/docs/react-api.html#reactmemo), and [`shouldComponentUpdate()`](https://reactjs.org/docs/react-component.html#shouldcomponentupdate) to prevent re-rendering expensive components. +- Using a combination of [React DevTools Profiler](https://chrome.google.com/webstore/detail/react-developer-tools/fmkadmapgofadopljbjfkapdkoienihi?hl=en) and [Chrome Dev Tools Performance Timing](https://calibreapp.com/blog/react-performance-profiling-optimization) can help identify unnecessary re-renders. Both tools can be used to time an interaction like the app starting up or navigating to a new screen. +- Watch out for [very large lists](https://reactnative.dev/docs/optimizing-flatlist-configuration) and things like `Image` components re-fetching images on render when a remote uri did not change. +- Avoid the temptation to over-optimize. There is added cost in both code complexity and performance when adding checks like `shouldComponentUpdate()`. Be selective about when you use this and make sure there is a measureable difference before proposing the change. As a very general rule it should be measurably faster to run logic to avoid the re-render (e.g. do a deep comparison) than it would be to let React take care of it without any extra intervention from us. +- Use caution when adding subscriptions that might re-render very large trees of components e.g. subscribing to state that changes often (current report, current route, etc) in the app root. +- Avoid using arrow function callbacks in components that are expensive to re-render. React will re-render this component since each time the parent renders it creates a new instance of that function. **Alternative:** Bind the method in the constructor instead. + +## Tools + +### Chrome Dev Tools > Performance > Timing (Web Only) + +- Profiling in Chrome Dev Tools performance tab in the "Timing" section +- This will show various components and how long they took to render. It can be a little intense to dig through it all at first, but the more time you spend with it the easier it gets to separate the signal from noise. +- The timing information might be inaccurate in development mode since this slows things down a ton. However, it's still useful for seeing which things take the longest and it's not too difficult to look and see which things are re-rendering. + +**Suggested:** [React Performance Profiling](https://calibreapp.com/blog/react-performance-profiling-optimization) + +### Hermes Profiling (Android only) + +It's possible, but slightly trickier to profile the JS running on Android devices as it does not run in a browser but a JS VM that React Native must spin up first then run the app code. The VM we are currently using on both Android and iOS is called [Hermes](https://reactnative.dev/docs/profile-hermes) and is developed by Facebook. + +In order to profile with Hermes follow these steps: + +- In the metro bundler window press `d` on your keyboard to bring up the developer menu on your device. +- Select "Settings" +- Select "Start Sampling Profiler on Init" +- In metro bundler refresh by pressing r +- The app will start up and a profile will begin +- Once the app loads take whatever action you want to profile +- Press `d` again and select "Disable Sampling Profiler" +- A toast should appear with a path to a profile +- We need to then convert this into something Chrome Dev Tools can use by typing into terminal `react-native profile-hermes .` +- This should create a json file in the directory where we typed the previous command that we can load up into Chrome Dev Tools "Performance" tab via the "Load Profile" option and inspect further. + +### React DevTools Profiler +- The React DevTools Profiler can also be used to detect similar information to Chrome Dev Tools, but is a little more streamlined. There is also an options cog where you can filter events by cutting at a specified millisecond (length it took for the thing to happen) +- Try checking the option to "Record why each component rendered while profiling". This may provide insights into why the component rendered unnecessarily. + +**Suggested:** [Deep Dive with the React DevTools creator](https://www.youtube.com/watch?v=nySib7ipZdk) + +### Why Did You Render? +- Why Did You Render (WDYR) sends console notifications about potentially avoidable component re-renders. +- It can also help to simply track when and why a certain component re-renders. +- To enable it, set `USE_WDYR=true` in your `.env` file. +- You can add or exclude tracked components by their `displayName` in `wdyr.js`. +- Open the browser console to see WDYR notifications. + +**Suggested** [Why Did You Render docs](https://github.com/welldone-software/why-did-you-render) + +### Performance Metrics (Opt-In on local release builds) + +To capture reliable performance metrics for native app launch we must test against a release build. To make this easier for everyone to do we created an opt-in tool (using [`react-native-performance`](https://github.com/oblador/react-native-performance) that will capture metrics and display them in an alert once the app becomes interactive. To set this up just set `CAPTURE_METRICS=true` in your `.env` file then create a release build on iOS or Android. The metrics this tool shows are as follows: + +- `nativeLaunch` - Total time for the native process to intialize +- `runJSBundle` - Total time to parse and execute the JS bundle +- `timeToInteractive` - Rough TTI (Time to Interactive). Includes native init time + sidebar UI partially loaded + +#### How to create a Release Build on Android + +- Create a keystore by running `keytool -genkey -v -keystore your_key_name.keystore -alias your_key_alias -keyalg RSA -keysize 2048 -validity 10000` +- Fill out all the prompts with any info and give it a password +- Drag the generated keystore to `/android/app` +- Hardcode the values to the gradle config like so: + +``` +signingConfigs { + release { + storeFile file('your_key_name.keystore') + storePassword 'Password1' + keyAlias 'your_key_alias' + keyPassword 'Password1' + } +``` +- Delete any existing apps off emulator or device +- Run `react-native run-android --variant release` + +## Reconciliation + +React is pretty smart and in many cases is able to tell if something needs to update. The process by which React goes about updating the "tree" or view heirarchy is called reconciliation. If React thinks something needs to update it will render it again. React also assumes that if a parent component rendered then it's child should also re-render. + +Re-rendering can be expensive at times and when dealing with nested props or state React may render when it doesn't need to which can be wasteful. A good example of this is a component that is being passed an object as a prop. Let's say the component only requires one or two properties from that object in order to build it's view, but doesn't care about some others. React will still re-render that component even if nothing it cares about has changed. Most of the time this is fine since reconciliation is pretty fast. But we might run into performance issues when re-rendering massive lists. + +In this example, the most preferable solution would be to **only pass the properties that the object needs to know about** to the component in the first place. + +Another option would be to use `shouldComponentUpdate` or `React.memo()` to add more specific rules comparing `props` to **explicitly tell React not to perform a re-render**. + +React might still take some time to re-render a component when it's parent component renders. If it takes a long time to re-render the child even though we have no props changing then we can use `PureComponent` or `React.memo()` (without a callback) which will "shallow compare" the `props` to see if a component should re-render. + +If you aren't sure what exactly is changing about some deeply nested object prop you can use `Performance.diffObject()` method in `componentDidUpdate()` which should show you exactly what is changing from one update to the next. + +**Suggested:** [React Docs - Reconciliation](https://reactjs.org/docs/reconciliation.html) diff --git a/web/data_flow.png b/docs/data_flow.png similarity index 100% rename from web/data_flow.png rename to docs/data_flow.png From 548c396a4d94a7be956c0156f1cba7cef7b713c8 Mon Sep 17 00:00:00 2001 From: Tim Golen Date: Mon, 4 Jul 2022 13:48:30 -0600 Subject: [PATCH 04/11] Move PR Review guidelines document --- .github/PULL_REQUEST_TEMPLATE.md | 8 ++-- PR_REVIEW_GUIDELINES.md | 73 +------------------------------- docs/PR_REVIEW_GUIDELINES.md | 72 +++++++++++++++++++++++++++++++ 3 files changed, 77 insertions(+), 76 deletions(-) create mode 100644 docs/PR_REVIEW_GUIDELINES.md diff --git a/.github/PULL_REQUEST_TEMPLATE.md b/.github/PULL_REQUEST_TEMPLATE.md index 90f42ac36bca..e1b9da03d646 100644 --- a/.github/PULL_REQUEST_TEMPLATE.md +++ b/.github/PULL_REQUEST_TEMPLATE.md @@ -51,7 +51,7 @@ This is a checklist for PR authors & reviewers. Please make sure to complete all - [ ] MacOS / Chrome - [ ] 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](https://github.com/Expensify/App/blob/main/PR_REVIEW_GUIDELINES.md#reviewing-the-code)) +- [ ] I followed proper code patterns (see [Reviewing the code](https://github.com/Expensify/App/blob/main/docs/PR_REVIEW_GUIDELINES.md#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. @@ -60,7 +60,7 @@ This is a checklist for PR authors & reviewers. Please make sure to complete all - [ ] 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`](https://github.com/Expensify/App/blob/main/STYLE.md#jsdocs)) 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](https://github.com/Expensify/App/blob/main/PR_REVIEW_GUIDELINES.md) +- [ ] I followed the guidelines as stated in the [Review Guidelines](https://github.com/Expensify/App/blob/main/docs/PR_REVIEW_GUIDELINES.md) - [ ] 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 @@ -100,7 +100,7 @@ This is a checklist for PR authors & reviewers. Please make sure to complete all - [ ] MacOS / Chrome - [ ] 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 verified proper code patterns were followed (see [Reviewing the code](https://github.com/Expensify/App/blob/main/PR_REVIEW_GUIDELINES.md#reviewing-the-code)) +- [ ] I verified proper code patterns were followed (see [Reviewing the code](https://github.com/Expensify/App/blob/main/docs/PR_REVIEW_GUIDELINES.md#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. @@ -109,7 +109,7 @@ This is a checklist for PR authors & reviewers. Please make sure to complete all - [ ] 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`](https://github.com/Expensify/App/blob/main/STYLE.md#jsdocs)) 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](https://github.com/Expensify/App/blob/main/PR_REVIEW_GUIDELINES.md) +- [ ] I verified that this PR follows the guidelines as stated in the [Review Guidelines](https://github.com/Expensify/App/blob/main/docs/PR_REVIEW_GUIDELINES.md) - [ ] 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: diff --git a/PR_REVIEW_GUIDELINES.md b/PR_REVIEW_GUIDELINES.md index 0470e7ceb31a..fd73c3864ba1 100644 --- a/PR_REVIEW_GUIDELINES.md +++ b/PR_REVIEW_GUIDELINES.md @@ -1,72 +1 @@ -# PR review guidelines - -This document lists specific guidelines for all PR reviews and aims to clarify items in the PR template checklist. - -# Check PR template - -1. Make sure the “Fixed Issues” section is formatted correctly. Links should be clickable and lead to the correct issue being fixed by this PR. -2. Make sure the testing steps are clear and correctly test the changes made. Make note of what’s required for the test account (i.e. do you need to test using a brand new account, have a workspace, have a bank account, etc). - - > **Rule of thumb**: Over-explain what the tester needs to do. Do not rely on the tester having existing knowledge of the app. - > - > ### Example of bad steps: - > - > 1. Go to this page - > 2. Click the button - > - > ### Example of good steps: - > - > 1. Log in with an account that has a workspace, or follow `[these steps](link-to-instructions-for setting-up-workspace)` to make one. - > 2. Click the settings button on the top left (the one with your avatar) - > 3. Navigate to “Your Workspace” > Reimburse Receipts > Connect bank account - > 4. Verify that the bank account view opens without errors - > - - Take note of the distinction between testing _locally_ and _on staging_. Note: The staging site references the production version of the API. -3. Make sure that the test includes both **success** and **fail** scenarios. - - Ex: if the issue was to prevent the user from saving a field when it is empty, besides testing that we should also test that they can save the field _if it is not empty_. - -# Testing the changes - -1. Make sure there are no console errors related to the PR. If you find console errors in a branch you’re reviewing, check if they also exist in `main`. - 1. If they do, proceed as normal (report them in #expensify-open-source if you think they need to be fixed). - 2. If the errors do not exist in `main`, report it to the PR author - they need to be fixed before the PR can be merged. -2. Test the changes on **all platforms**. Follow this guide (needs link) to set up each platform. - - If you’re unable to boot a platform for any reason, ask for help in the #expensify-open-source Slack channel. Your issue might already have been addressed there, so be sure to search first. - -# Reviewing the code - -## Good code patterns to require - -1. Check that functions have comments when appropriate. - - If the function has params or returns something, use [JSDocs syntax]((https://github.com/Expensify/App/blob/main/STYLE.md#jsdocs)) to describe them. - - Indicate the param name(s), data type passed and / or returned, and purpose if not immediately obvious. - - Obvious functions (with no params / return value) should not have comments. - - **In short: _Add comments & docs, only when useful._** -2. All copy / text shown to users in the product should be stored in the `src/languages/*` files for localization. -3. Platform-specific files should follow the proper naming convention (see [Platform-Specific File Extensions](https://github.com/expensify/app#platform-specific-file-extensions)) - - Example: `MyComponent/index.android.js` - -## Code patterns to improve - -1. Platform-specific solutions should not be applied to more platforms than necessary - - E.g. solutions for Android or iOS should go in specific `index.android.js` or `index.ios.js` files, not in an `index.native.js` file. -2. Were any new styles added? Are we following the guidelines in [`STYLING.md`](./STYLING.md)? - -## Code patterns to avoid - -1. Make sure any `setTimeout`s are absolutely necessary, not just a workaround for a situation that isn’t fully understood (e.g. a race condition). -2. Ensure Onyx is used appropriately. ([docs](https://github.com/expensify/react-native-onyx#merging-data)) - -## Considerations around code reuse - -While trying to keep our code DRY (don't repeat yourself), you may find yourself trying to decide between refactoring & reusing a component vs. creating a brand new component. When in doubt about whether something should be reused or refactored, ask for help. - -Here are some guidelines we use in order to make the best decisions: - -1. Specialization - - When one component is a special case of another, we should opt for the technique of composition. Here are the [React docs](https://reactjs.org/docs/composition-vs-inheritance.html#specialization) on Specialization. The idea is that it’s better to establish a pattern of creating increasingly specific components, instead of adding dozens of use cases to a single component. - - You might consider this when a component is being reused because the new use case is “close enough” to the original. Rather than adding functionality to that component with subtle additions, it may be better to create a new, more specialized version of that component. -1. Refactors - - During a code review it often becomes apparent that a refactor is needed. In this case, we recommend following these steps: - 1. Identify the refactor and confirm with others that it’s needed. - 2. Do that refactor first (as a separate job, if it qualifies), merge it, test for regressions. - 3. Proceed with the original issue & job. +This file has been moved [here](./docs/PR_REVIEW_GUIDELINES.md). diff --git a/docs/PR_REVIEW_GUIDELINES.md b/docs/PR_REVIEW_GUIDELINES.md new file mode 100644 index 000000000000..0470e7ceb31a --- /dev/null +++ b/docs/PR_REVIEW_GUIDELINES.md @@ -0,0 +1,72 @@ +# PR review guidelines + +This document lists specific guidelines for all PR reviews and aims to clarify items in the PR template checklist. + +# Check PR template + +1. Make sure the “Fixed Issues” section is formatted correctly. Links should be clickable and lead to the correct issue being fixed by this PR. +2. Make sure the testing steps are clear and correctly test the changes made. Make note of what’s required for the test account (i.e. do you need to test using a brand new account, have a workspace, have a bank account, etc). + - > **Rule of thumb**: Over-explain what the tester needs to do. Do not rely on the tester having existing knowledge of the app. + > + > ### Example of bad steps: + > + > 1. Go to this page + > 2. Click the button + > + > ### Example of good steps: + > + > 1. Log in with an account that has a workspace, or follow `[these steps](link-to-instructions-for setting-up-workspace)` to make one. + > 2. Click the settings button on the top left (the one with your avatar) + > 3. Navigate to “Your Workspace” > Reimburse Receipts > Connect bank account + > 4. Verify that the bank account view opens without errors + > + - Take note of the distinction between testing _locally_ and _on staging_. Note: The staging site references the production version of the API. +3. Make sure that the test includes both **success** and **fail** scenarios. + - Ex: if the issue was to prevent the user from saving a field when it is empty, besides testing that we should also test that they can save the field _if it is not empty_. + +# Testing the changes + +1. Make sure there are no console errors related to the PR. If you find console errors in a branch you’re reviewing, check if they also exist in `main`. + 1. If they do, proceed as normal (report them in #expensify-open-source if you think they need to be fixed). + 2. If the errors do not exist in `main`, report it to the PR author - they need to be fixed before the PR can be merged. +2. Test the changes on **all platforms**. Follow this guide (needs link) to set up each platform. + - If you’re unable to boot a platform for any reason, ask for help in the #expensify-open-source Slack channel. Your issue might already have been addressed there, so be sure to search first. + +# Reviewing the code + +## Good code patterns to require + +1. Check that functions have comments when appropriate. + - If the function has params or returns something, use [JSDocs syntax]((https://github.com/Expensify/App/blob/main/STYLE.md#jsdocs)) to describe them. + - Indicate the param name(s), data type passed and / or returned, and purpose if not immediately obvious. + - Obvious functions (with no params / return value) should not have comments. + - **In short: _Add comments & docs, only when useful._** +2. All copy / text shown to users in the product should be stored in the `src/languages/*` files for localization. +3. Platform-specific files should follow the proper naming convention (see [Platform-Specific File Extensions](https://github.com/expensify/app#platform-specific-file-extensions)) + - Example: `MyComponent/index.android.js` + +## Code patterns to improve + +1. Platform-specific solutions should not be applied to more platforms than necessary + - E.g. solutions for Android or iOS should go in specific `index.android.js` or `index.ios.js` files, not in an `index.native.js` file. +2. Were any new styles added? Are we following the guidelines in [`STYLING.md`](./STYLING.md)? + +## Code patterns to avoid + +1. Make sure any `setTimeout`s are absolutely necessary, not just a workaround for a situation that isn’t fully understood (e.g. a race condition). +2. Ensure Onyx is used appropriately. ([docs](https://github.com/expensify/react-native-onyx#merging-data)) + +## Considerations around code reuse + +While trying to keep our code DRY (don't repeat yourself), you may find yourself trying to decide between refactoring & reusing a component vs. creating a brand new component. When in doubt about whether something should be reused or refactored, ask for help. + +Here are some guidelines we use in order to make the best decisions: + +1. Specialization + - When one component is a special case of another, we should opt for the technique of composition. Here are the [React docs](https://reactjs.org/docs/composition-vs-inheritance.html#specialization) on Specialization. The idea is that it’s better to establish a pattern of creating increasingly specific components, instead of adding dozens of use cases to a single component. + - You might consider this when a component is being reused because the new use case is “close enough” to the original. Rather than adding functionality to that component with subtle additions, it may be better to create a new, more specialized version of that component. +1. Refactors + - During a code review it often becomes apparent that a refactor is needed. In this case, we recommend following these steps: + 1. Identify the refactor and confirm with others that it’s needed. + 2. Do that refactor first (as a separate job, if it qualifies), merge it, test for regressions. + 3. Proceed with the original issue & job. From 882680d0a8741ec588cffb8e223a9e2da2f1a74a Mon Sep 17 00:00:00 2001 From: Tim Golen Date: Mon, 4 Jul 2022 13:49:26 -0600 Subject: [PATCH 05/11] Move storybook document --- STORYBOOK.md | 72 +---------------------------------------------- docs/STORYBOOK.md | 71 ++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 72 insertions(+), 71 deletions(-) create mode 100644 docs/STORYBOOK.md diff --git a/STORYBOOK.md b/STORYBOOK.md index cb4cf290a77a..9f4a3c1f6c1c 100644 --- a/STORYBOOK.md +++ b/STORYBOOK.md @@ -1,71 +1 @@ -## Storybook - ->Storybook is an open source tool for building UI components and pages in isolation. It streamlines UI development, testing, and documentation. - -At Expensify, we primarily use [Storybook](https://storybook.js.org/) to provide interactive documentation of our design system. This helps bridge the gap between design and engineering to encourage code reusibility and improve communication. - -### Building and Deploying Storybook - -The Storybook docs deploy automatically so there's nothing extra to do here. Storybook is built to the `/dist` folder and lives at [`https://new.expensify.com/docs/index.html`](https://new.expensify.com/docs/index.html). - -To test a local build we can run - -``` -npm run storybook-build -``` - -This will create a `/docs` directory in `/dist` with the static site. - -### Local Testing - -To skip building and load Storybook on a local server run: - -``` -npm run storybook -``` - -### What's a "story" and how do I create one? - ->A story captures the rendered state of a UI component. Developers write multiple stories per component that describe all the “interesting” states a component can support. - -All of our stories are located in a single directory `/src/stories`. - -To create a new story for a component that does not yet have one it's a good idea to copy an existing story file and then update the relevant details. We are using the [Component Story Format](https://storybook.js.org/docs/react/writing-stories/introduction#component-story-format) to write our stories. - -Here's an example story: - -```javascript -import React from 'react'; -import Button from '../components/Button'; - -const story = { - // Title field will determine how the story displays in the sidebar - title: 'Components/Button', - component: Button, -}; - -// Optional `args` are passed which determine the props the example component will have -const Template = args =>