Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add STYLING.md to clarify styling best practices #2185

Merged
merged 7 commits into from
Apr 26, 2021
Merged

Add STYLING.md to clarify styling best practices #2185

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
bdeaa69
Add styling.md
marcaaron Mar 31, 2021
831ca61
mention helper styles
marcaaron Mar 31, 2021
fecc1fe
fix language
marcaaron Mar 31, 2021
f76afef
add note about bootstrap
marcaaron Mar 31, 2021
f10089a
Add some info about elementModifier convention
marcaaron Apr 20, 2021
689bae5
Update style definition and style props sections
marcaaron Apr 20, 2021
0467521
add note about helper functions and modules
marcaaron Apr 22, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 14 additions & 3 deletions STYLE.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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`
Expand Down Expand Up @@ -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 <View ref={el => this.myRef = el} />;
}
Expand All @@ -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;
}
Expand Down
216 changes: 216 additions & 0 deletions STYLING.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,216 @@
# React Native Styling Guidelines

## Where to Define Styles

All styles must be defined in the `/styles` directory and `styles.js` contains the final export after gathering all appropriate styles. Unlike some React Native applications we are not using `StyleSheet.create()` and instead store styles as plain JS objects. There are also many helper styles available for direct use in components.

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 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
const TextWithPadding = props => (
<Text style={styles.textWithPadding}>
{props.children}
</Text>
);

// Good
const TextWithPadding = props => (
<Text
style={[
styles.p5,
styles.noWrap,
]}
>
{props.children}
</Text>
);
```

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"

In order to resist the urge to preoptimize and have many single-use components we've adopted a main principle:

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 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
<Button title="Submit" success large />
```

## Inline Styles

**Inline styles are forbidden.** If we run into a case where we feel it's necessary to conditionally render some styles we should create a helper function then pass any modifying parameters to that function. Small helper functions can be written directly in `styles.js`, but larger, more complex methods should be put in their own modules and imported into `styles.js`.

```jsx
// Bad - Do not use inline styles
const TextWithPadding = props => (
<Text style={{
padding: 10,
whiteSpace: props.shouldWrap ? 'wrap' : 'nowrap',
}}>
{props.children}
</Text>
);

// Good
const TextWithPadding = props => (
<Text
style={[
styles.p5,
getTextWrapStyle(props.shouldWrap)
]}
>
{props.children}
</Text>
);
```

## How to Reuse Styles

There are many styles in the `styles.js` file. It is generally a bad practice to grab a style meant for a _specific_ use case and utilize it for some other more _general_ 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 => (
<View>
<Text style={[styles.settingsScreenText]}>
Expensify
</Text>
</View>
);

const SomeOtherScreen = props => (
<View>
<Text style={[styles.settingsScreenText]}>
Expensify.cash
</Text>
</View>
);

// Good
const SettingsScreen = props => (
<View>
<Text style={[styles.defaultScreenText]}>
Expensify
</Text>
</View>
);

const SomeOtherScreen = props => (
<View>
<Text style={[styles.defaultScreenText]}>
Expensify.cash
</Text>
</View>
);
```

## 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.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is all stemming from this conversation -> #1878 (comment)

I'm cool with whatever we decide to do as long as it's consistent

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, as stated in that other thread, I think that having the extra handling where a style can be an object or an array will be simpler than actually tying it to the plurality of the prop name. I have three reasons:

  1. Built-in React Native components such as View and Pressable can take either and object or array of styles, so having our components follow the same pattern is a good thing for consistency.
  2. React native provides a pretty easy way to deal with styles that can be an array or an object in StyleSheet.flatten, and I think that's a pretty easy pattern to pick up on and remember. "If you have a style prop, flatten it!" The docs make me think there might be some performance implications of overusing that though, so maybe the tertiary pattern is better 🤷🏼‍♂️
  3. When dealing with contributors (esp. those for whom English is a second language) and reviewing PRs, I just have a feeling that this style == Object and styles == Array might slip through the cracks and end up not being thoroughly enforced.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice thoughts.

I guess my concern with making all components behave like React Native components is that we'll always have to use a bunch of StyleSheet.flatten() everywhere and it makes adding styles to existing default styles kind of tricky.

Maybe if we are going to standardize on something we can just do "always use an array"?

If extra style props are always arrays then we can spread them or pass them directly and don't have to handle different types.

Copy link
Contributor

Choose a reason for hiding this comment

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

I like the idea of always standardizing on arrays. I think that solves the problem pretty nicely and makes it very clear how something should be done.

Copy link
Contributor

Choose a reason for hiding this comment

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

Always using arrays is fine with me.


### Complex Component

Always pass style props with a name that describes which child element styles will be modified. All style props should accept an `Array` of style `Object` and have a pluralized name e.g. `headerStyles`

```jsx
// Bad - props.style should not be used in complex components
const SettingsScreen = props => (
<View>
<Header
style={[
styles.defaultHeader,
props.style,
]}
/>
<Body style={props.bodyStyles} />
...
</View>
);

// Bad - style with a flexible type requires extra handling
const SettingsScreen = props => {
const extraHeaderStyles = _.isArray(props.headerStyle)
? props.headerStyle
: [props.headerStyle];
return (
<View>
<Header
style={[
styles.defaultHeader,
...extraHeaderStyles,
]}
/>
<Body style={[props.bodyStyle]} />
...
</View>
);
}

// Bad - Uses a singular and passes a single style object
const SettingsScreen = props => (
<View>
<Header
style={[
styles.defaultHeader,
props.headerStyle,
]}
/>
...
</View>
);

// Good - Uses a plural and passes an array of style objects with spread syntax
const SettingsScreen = props => (
<View>
<Header
style={[
styles.defaultHeader,
...props.headerStyles,
]}
/>
...
</View>
);
```

### 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 => (
<Text style={props.style}>{props.children}</Text>
);

// Good
const CustomText = props => {
const propsStyle = _.isArray(props.style)
? props.style
: [props.style];
}(
<Text
style={[
styles.defaultCustomText,
...propsStyle,
]}
>
{props.children}
</Text>
);
```

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.