Add STYLING.md to clarify styling best practices

[marcaaron]

Details

cc @roryabraham this is a first draft of some general styling practices so that contributors (and us) have an agreed upon way of doing things.

also wanted to get some input from @shawnborton on some of this stuff.

I have the feeling that some of the styling stuff is getting away from us and want to establish some baseline best practices before things turn into total chaos.

Fixed Issues

Fixes https://github.com/Expensify/Expensify/issues/161044

Tests

N/A

QA Steps

N/A

Tested On

N/A

[marcaaron]

I feel like this is mostly correct based on a lot of what I'm seeing so far. But maybe @shawnborton has some thoughts on this.

Another thought I had was that maybe it's OK to move an array of styles to styles.something just from a code organization perspective. Since otherwise we'd have these massive lists inline arrays of helpers everywhere (which is kind of where we're headed now).

Still, it's difficult at times to tell whether to create a new style in styles.js or to use a big array of helpers.

[roryabraham]

I personally prefer to see a new style created whenever we're composing more than two or three styles inline (especially if none of those styles are conditional). My reasoning is that we don't want to muddle our JSX with too much style logic inline, because I think it reduces readability of the code. And composing 4 new styles is basically equivalent to creating a new one. Even in style.js, however, we can take advantage of all the utilities such as our color themes, spacing, fonts, flex, etc... that are nice to be able to adjust globally.

Maybe we could provide some concrete guidelines by imposing a rule-of-three here and agree concretely that composing three styles inline is acceptable, but more than that should be created in a separate file?

[marcaaron]

Thanks. I think this is probably the most contentious thing I want to introduce here - but mostly providing this as a starting point. I think the suggestion to have no more than inline 3 helpers is interesting, but I'm not totally confident about it. I worry it can lead to lots of "one off" styles that are never re-used again. But maybe that's OK?

Would love to get @shawnborton's take on this if he's got some perspective to share that might help us settle on something. I'm not 100% sure what would be best and would like to lean on his experience to help establish a baseline.

[shawnborton]

I think if the component will be a truly reusable component, it makes sense to try to put all of the styles together under one "class." Like take buttons for example, we don't want someone to need to know to use the right border radius, background color, font color, and font size. So I actually think I lean a little bit towards the opposite of what you are saying.

Now I do think we should encourage people to use helper styles for things like spacing, layout, flex, etc. But any kind of component that will be reused multiple times, let's give it a class. But maybe one thing to distinguish is that each reusable component is essentially made up of other smaller components. So you should always use (existing) variables for things like font size, colors, radius, etc. Thoughts on that?

[marcaaron]

Sorry for the late reply here @shawnborton.

we should encourage people to use helper styles for things like spacing, layout, flex, etc. But any kind of component that will be reused multiple times, let's give it a class.

I think you are essentially saying we should do a "class" or style definition over a React component, but want to clarify since "component" could mean different things to different people.

If we could distill what you are saying into some kind of rule what would it be?

I lean a little bit towards the opposite of what you are saying

It sounds like we are generally agreeing that "single use styles" are bad but not sure which thing you are opposed to, so let us know.

[shawnborton]

Yeah, maybe we are actually agreeing on most of this. I agree that single use styles are bad, we want people to look at the existing design system and reuse what's there if possible rather than creating something new for a one-off scenario.

I think you are essentially saying we should do a "class" or style definition over a React component, but want to clarify since "component" could mean different things to different people.

Totally, from a non-technical perspective, I am saying that that any kind of UI object (button, form, chat row, etc) that will be reused, let's encapsulate it into one object that has additional modifiers. Let me know if that generally makes sense.

[marcaaron]

Nice. So, by encapsulate into one object that has additional modifiers do we just mean adopt some naming convention where the modifiers are also "classes"? Probably the button* styles are the clearest example of where we've begun to do something like this...

const styles = {
    button: {},
    buttonSuccess: {},
    buttonDisable: {},
    buttonSmallText: {},
}

[shawnborton]

Yup I think that's a great example. Though in the future, I think the button component would be a great candidate for some kind of prop/value convention where we'd have something roughly like <button size=small style=secondary styles={anything extra could go here like width utilities, etc}>

[marcaaron]

Cool, yeah I agree with the prop/value convention. Part of my motivation with adding the STYLING.md stuff is to maybe codify these ideas into a set of rules so we have an easy way to guide people on how to approach styles and refactorings...

What if we did something like:

• If an array of styles is used 3 times it should be given it's own style object e.g. styles.button
• Variations on that style should follow a naming convention of styles.componentModifer e.g. styles.buttonSuccess
• If a major style has more than 3 usages and also multiple modifier styles it should be moved into a re-usable component so styles can be modified via props

I think that might be a good framework to start with ?

[shawnborton]

That sounds great to me.

[marcaaron]

Open to other interpretations on this. "Rule of Three" seems pretty simple to me and helps shut down disagreements about what is expected.

Less sure about a guiding principle for when we should create a new component vs. a new style object

[marcaaron]

We could probably also just settle on one of these options so that the guidance is clearer when this pops up. I think the helper function style makes the most sense.

[tgolen]

I'm happy to settle on just using helper methods, but I'm not opposed to leaving both methods for now (I'm not sure how much this happens).

[marcaaron]

Not too often but often enough that we will have tons eventually. But yeah I think it's fine to promote the helper method for now and will simplify the guidance a bit.

[marcaaron]

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

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

[roryabraham]

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.

[marcaaron]

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.

[tgolen]

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.

[roryabraham]

Always using arrays is fine with me.

[roryabraham]

I've also used a pattern a few times of creating functions in a style file to create dynamic styles like here and here. What are our thoughts on that? I sort of think it's fine, but we could also move those same functions into styles.js.

[roryabraham]

Some similar functions already live in styles.js here, so we haven't been consistent there.

I really don't have a preference whether those dynamic style generators live in their own files or styles.js, but we should be consistent.

[marcaaron]

Yeah I think those function files can be modular but maybe we enforce that they are imported via styles.js directly so they are all documented there at least?

[tgolen]

I like the idea of modularizing those methods better. I think that style.js should just be in charge of collecting and being the final export of our styles. There are a couple minor changes I'd like to request for this paragraph.

1. All styles must be defined in the /styles directory and style.js contains the final export after gathering all appropriate styles.
2. Remove "of sorts"

[marcaaron]

@tgolen curious to get your thoughts on these proposed changes if you have some time

[tgolen]

I love where this is heading!

[tgolen]

I like the idea of modularizing those methods better. I think that style.js should just be in charge of collecting and being the final export of our styles. There are a couple minor changes I'd like to request for this paragraph.

1. All styles must be defined in the /styles directory and style.js contains the final export after gathering all appropriate styles.
2. Remove "of sorts"

[tgolen]

I'm happy to settle on just using helper methods, but I'm not opposed to leaving both methods for now (I'm not sure how much this happens).

[tgolen]

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.

[marcaaron]

Thanks @tgolen. Made some updates based on your feedback.

[roryabraham]

create a helper function and import it into styles.js

So we decided to have dynamic style generator functions live in separate files, like getModalStyles.js, instead of putting those functions directly in styles.js? It's fine with me, just wanted to clarify that this is the intention.

[marcaaron]

I think they can live directly in styles.js unless they are very large and there is some necessity to create a new file? But don't really have strong feelings on this point.

[tgolen]

I'd like to avoid having a bunch of one-teeny-method-files if we can. So for smaller helpers, I think having them in style.js is fine. More complex ones are fine to put into their own modules.

[roryabraham]

Cool, I agree. Maybe we can add a sentence like:

"Small helper functions can be written directly in styles.js, but larger, more complex dynamic style generators can be put in their own modules"

[roryabraham]

Always using arrays is fine with me.

[marcaaron]

Gonna merge this one since I don't think there were any other blockers and it will be good to have some styling notes to pass along to contributors in the future.

[OSBotify]

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

[OSBotify]

🚀 Deployed to staging in version: 1.0.31-1🚀

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

[OSBotify]

🚀 Deployed to production in version: 1.0.39-5🚀

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