From bdeaa699f0602c8aa4e04bb2397d95e07678be8f Mon Sep 17 00:00:00 2001 From: Marc Glasser Date: Wed, 31 Mar 2021 13:08:27 -1000 Subject: [PATCH 1/7] Add styling.md --- STYLE.md | 17 ++++- STYLING.md | 221 +++++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 235 insertions(+), 3 deletions(-) create mode 100644 STYLING.md diff --git a/STYLE.md b/STYLE.md index 3850c5463239..e3cb20ca5dee 100644 --- a/STYLE.md +++ b/STYLE.md @@ -141,6 +141,17 @@ Using arrow functions is the preferred way to write an anonymous function such a _.map(someArray, (item) => {...}); ``` +Empty functions (noop) should be declare as arrow functions with no whitespace inside. Avoid _.noop() + + ```javascript + // Bad + const callback = _.noop; + const callback = () => { }; + + // Good + const callback = () => {}; + ``` + ## `var`, `const` and `let` - Never use `var` @@ -655,10 +666,10 @@ In addition, all refs should be declared in the constructor, rather than inline. class BadRefComponent extends Component { constructor(props) { super(props); - + // Bad: Ref is declared inline instead of in the constructor } - + render() { return this.myRef = el} />; } @@ -668,7 +679,7 @@ class BadRefComponent extends Component { class GoodRefComponent extends Component { constructor(props) { super(props); - + // Good: Ref is declared in the constructor this.myRef = undefined; } diff --git a/STYLING.md b/STYLING.md new file mode 100644 index 000000000000..81954e779555 --- /dev/null +++ b/STYLING.md @@ -0,0 +1,221 @@ +# React Native Styling Guidelines + +## Where to Define Styles + +All styles must be defined in the `styles.js` file which exists as a globally exported stylesheet of sorts for the application. Unlike some React Native applications we are not using `StyleSheet.create()` and instead store styles as plain JS objects. There are many helper styles that should be used in components. + +## When to Create a New Style + +If we need some minimal style changes then it's almost always better to use a collection of helper styles rather than create an entirely new style if it will only be used once. Resist the urge to create a new style for any new element added to a screen. There is a very good chance the style we are adding is a "single-use" style. + +```jsx +// Bad - Since we only use this style once in this component +const TextWithPadding = props => ( + + {props.children} + +); + +// Good +const TextWithPadding = props => ( + + {props.children} + +); +``` + +On the other hand, if we are copying and pasting some chunks of JSX from one place to another then that might be a sign that we need either a new component or a new reusable style. + +## Use the "Rule of Three" + +Any array of styles associated with a single type of component or element that has at least 3 identical usages should be refactored into either: + +- A new component with that same array of helper styles + +- A new style that is a composite of all helper styles + +## Inline Styles + +**Inline styles are forbidden.** If we run into a case where we feel it's necessary to conditionally render some styles. There are two options: + +1. Create a helper function in `styles.js` and pass any modifying parameters to that function. +1. Conditionally render a style object inline by defaulting to `undefined`. + +```jsx +// Bad - Do not use inline styles +const TextWithPadding = props => ( + + {props.children} + +); + +// Good +const TextWithPadding = props => ( + + {props.children} + +); + +// Good +const TextWithPadding = props => ( + + {props.children} + +); +``` + +## Don't Go Style Fishing + +There are many styles in the `styles.js` file. It is generally a bad practice to grab style meant for a specific use case and utilize it for some other use case without changing it's name to make it more general. If we think we see a style that might be appropriate for reuse, but does not have a generic name then we should rename it instead of using it directly. + +```jsx +// Bad - Reuses style without generalizing style name +const SettingsScreen = props => ( + + + Expensify + + +); + +const SomeOtherScreen = props => ( + + + Expensify.cash + + +); + +// Good +const SettingsScreen = props => ( + + + Expensify + + +); + +const SomeOtherScreen = props => ( + + + Expensify.cash + + +); +``` + +## When and How to Pass Styles via Props + +In some cases, we may want a more complex component to allow a parent to modify a style of one of it's child elements. In other cases, we may have a very simple component that has one child which has a `style` prop. Let's look at how to handle these two examples. + +### Complex Component + +Always pass style props with clear names that describe which child element style will be modified. Specific styles should clearly indicate what the expected type is by using a singular (`Object`) or plural (`Array`) naming convention. + +```jsx +// Bad - props.style should not be used in complex components +const SettingsScreen = props => ( + +
+ + ... + +); + +// Bad - style with a flexible type requires extra handling +const SettingsScreen = props => { + const extraHeaderStyles = _.isArray(props.headerStyle) + ? props.headerStyle + : [props.headerStyle]; + return ( + +
+ + ... + + ); +} + +// Good - Uses a singular and passes a single style object +const SettingsScreen = props => ( + +
+ ... + +); + +// Good - Uses a plural and passes an array of style objects with spread syntax +const SettingsScreen = props => ( + +
+ ... + +); +``` + +### Simple Component + +The only time we should allow a component to have a `style` prop with `PropTypes.any` is when we are wrapping a single child that has a flexible `style` type that accepts both `Array` or `Object` types. + +```jsx +// Good +const CustomText = props => ( + {props.children} +); + +// Good +const CustomText = props => { + const propsStyle = _.isArray(props.style) + ? props.style + : [props.style]; +}( + + {props.children} + +); +``` + +In that last example, there is just one simple element and no ambiguity about what `props.style` refers to. The component is used in many places and has some default styles therefore we must add custom style handling behavior. \ No newline at end of file From 831ca61279fdbee1ee266c1619baab76414dd182 Mon Sep 17 00:00:00 2001 From: Marc Glasser Date: Wed, 31 Mar 2021 13:12:10 -1000 Subject: [PATCH 2/7] mention helper styles --- STYLING.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/STYLING.md b/STYLING.md index 81954e779555..4a1c31d6a485 100644 --- a/STYLING.md +++ b/STYLING.md @@ -2,11 +2,11 @@ ## Where to Define Styles -All styles must be defined in the `styles.js` file which exists as a globally exported stylesheet of sorts for the application. Unlike some React Native applications we are not using `StyleSheet.create()` and instead store styles as plain JS objects. There are many helper styles that should be used in components. +All styles must be defined in the `styles.js` file which exists as a globally exported stylesheet of sorts for the application. Unlike some React Native applications we are not using `StyleSheet.create()` and instead store styles as plain JS objects. There are many helper styles available for direct use in components and can be found in the `/styles` directory. ## When to Create a New Style -If we need some minimal style changes then it's almost always better to use a collection of helper styles rather than create an entirely new style if it will only be used once. Resist the urge to create a new style for any new element added to a screen. There is a very good chance the style we are adding is a "single-use" style. +If we need some minimal style changes then it's almost always better to use an array of helper styles rather than create an entirely new style if it will only be used once. Resist the urge to create a new style for any new element added to a screen. There is a very good chance the style we are adding is a "single-use" style. ```jsx // Bad - Since we only use this style once in this component From fecc1feca9f62cf0135d7351220eb80a288b829d Mon Sep 17 00:00:00 2001 From: Marc Glasser Date: Wed, 31 Mar 2021 13:13:14 -1000 Subject: [PATCH 3/7] fix language --- STYLING.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/STYLING.md b/STYLING.md index 4a1c31d6a485..1ecd402afdfb 100644 --- a/STYLING.md +++ b/STYLING.md @@ -6,7 +6,7 @@ All styles must be defined in the `styles.js` file which exists as a globally ex ## When to Create a New Style -If we need some minimal style changes then it's almost always better to use an array of helper styles rather than create an entirely new style if it will only be used once. Resist the urge to create a new style for any new element added to a screen. There is a very good chance the style we are adding is a "single-use" style. +If we need some minimal set of styling rules applied to a component then it's almost always better to use an array of helper styles rather than create an entirely new style if it will only be used once. Resist the urge to create a new style for any new element added to a screen. There is a very good chance the style we are adding is a "single-use" style. ```jsx // Bad - Since we only use this style once in this component From f76afef33c9eecb8983f5cf2de5ffb9cd511dcd8 Mon Sep 17 00:00:00 2001 From: Marc Glasser Date: Wed, 31 Mar 2021 13:16:45 -1000 Subject: [PATCH 4/7] add note about bootstrap --- STYLING.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/STYLING.md b/STYLING.md index 1ecd402afdfb..1025081f4f22 100644 --- a/STYLING.md +++ b/STYLING.md @@ -4,6 +4,10 @@ All styles must be defined in the `styles.js` file which exists as a globally exported stylesheet of sorts for the application. Unlike some React Native applications we are not using `StyleSheet.create()` and instead store styles as plain JS objects. There are many helper styles available for direct use in components and can be found in the `/styles` directory. +These helper styles are loosely based on the [Bootstrap system of CSS utility helper classes](https://getbootstrap.com/docs/5.0/utilities/spacing/) and are typically incremented by units of 4. + +**Note:** Not all helpers from Bootstrap exist, so it may be necessary to create the helper style we need. + ## When to Create a New Style If we need some minimal set of styling rules applied to a component then it's almost always better to use an array of helper styles rather than create an entirely new style if it will only be used once. Resist the urge to create a new style for any new element added to a screen. There is a very good chance the style we are adding is a "single-use" style. From f10089acf5544ee28c75f0113ecc24dc70083ca2 Mon Sep 17 00:00:00 2001 From: Marc Glasser Date: Tue, 20 Apr 2021 07:14:16 -1000 Subject: [PATCH 5/7] Add some info about elementModifier convention --- STYLING.md | 24 +++++++++++++++--------- 1 file changed, 15 insertions(+), 9 deletions(-) diff --git a/STYLING.md b/STYLING.md index 1025081f4f22..54d2b06bd27d 100644 --- a/STYLING.md +++ b/STYLING.md @@ -4,13 +4,13 @@ All styles must be defined in the `styles.js` file which exists as a globally exported stylesheet of sorts for the application. Unlike some React Native applications we are not using `StyleSheet.create()` and instead store styles as plain JS objects. There are many helper styles available for direct use in components and can be found in the `/styles` directory. -These helper styles are loosely based on the [Bootstrap system of CSS utility helper classes](https://getbootstrap.com/docs/5.0/utilities/spacing/) and are typically incremented by units of 4. +These helper styles are loosely based on the [Bootstrap system of CSS utility helper classes](https://getbootstrap.com/docs/5.0/utilities/spacing/) and are typically incremented by units of `4`. **Note:** Not all helpers from Bootstrap exist, so it may be necessary to create the helper style we need. ## When to Create a New Style -If we need some minimal set of styling rules applied to a component then it's almost always better to use an array of helper styles rather than create an entirely new style if it will only be used once. Resist the urge to create a new style for any new element added to a screen. There is a very good chance the style we are adding is a "single-use" style. +If we need some minimal set of styling rules applied to a single-use component then it's almost always better to use an array of helper styles rather than create an entirely new style if it will only be used once. Resist the urge to create a new style for every new element added to a screen. There is a very good chance the style we are adding is a "single-use" style. ```jsx // Bad - Since we only use this style once in this component @@ -33,15 +33,21 @@ const TextWithPadding = props => ( ); ``` -On the other hand, if we are copying and pasting some chunks of JSX from one place to another then that might be a sign that we need either a new component or a new reusable style. +On the other hand, if we are copying and pasting some chunks of JSX from one place to another then that might be a sign that we need a new reusable style. ## Use the "Rule of Three" -Any array of styles associated with a single type of component or element that has at least 3 identical usages should be refactored into either: +In order to resist the urge to preoptimize and have many single-use components we've adopted a main principle: -- A new component with that same array of helper styles +Any array of styles associated with a single type of React element that has at least 3 identical usages should be refactored into: -- A new style that is a composite of all helper styles +- A new resusable style that can be used in many places e.g. `styles.button` +- If that style has modifiers or style variations then those styles should follow a naming convention of `styles.elementModifer` e.g. `styles.buttonSuccess` +- If a reusable style has 3 or more modifiers it should be refactored into a component with props to modify the styles e.g. + +```jsx +