Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Add styled components - Let's talk #480

Closed
alejandronanez opened this issue Oct 15, 2017 · 22 comments
Closed

Add styled components - Let's talk #480

alejandronanez opened this issue Oct 15, 2017 · 22 comments

Comments

@alejandronanez
Copy link
Member

alejandronanez commented Oct 15, 2017

Add styled components - PoC

Update

Let's talk about what is "wrong" / "right" about our current implementation for styling, and what styled-components will bring to the table. If we're not going to see any real benefit I think we should hold this off.

Update 2

There's a list here #532 with all the components that need styling, feel free to grab the component you want to work on.

@alejandronanez alejandronanez self-assigned this Oct 15, 2017
@alejandronanez alejandronanez changed the title Add styled components Add styled components - Let's talk Oct 15, 2017
@lex111
Copy link
Member

lex111 commented Oct 15, 2017

and what styled-components will bring to the table

Honestly, I do not understand how this will help? Can you explain the pros and cons?

@alejandronanez
Copy link
Member Author

alejandronanez commented Oct 15, 2017

@lex111 well this is what this issue is for. We talked about this briefly on Gitter so I wanted to move this here so everybody can share their opinion. To evaluate if it's worth it to add styled-components or not.

@andrewda
Copy link
Member

Disclaimer: I've barely used styled-components, and most of what I know is from the documentation.

Current Styling

Pros

1. Concise

All of our styles are already in their respective component file meaning we don't have any extra styling-only files.

2. Explicitly showing styles

The current implementation is easy to follow because we provide every component that needs a style with a style property.

Cons

1. Too concise

Everything is in one file. This can be good (see Pro 1), but can also lead to readability issues. Cutting out the styles into new files might be beneficial.

2. Using StyleSheet.flatten()

We're using StyleSheet.flatten() in several places, which isn't always super nice. It would be good to try to avoid using this, as it can lead to confusing styles, especially when they begin to override each other.

Styled Components

Pros

1. Easy linting

This looks like one of the biggest pros. Keep our styling in a consistent format with their super nice linting tool.

2. Writing actual CSS

CSS in objects can get a little weird, especially when it comes to using dashes. With styled-components, we're writing actual CSS instead of weird object CSS.

3. Syntax highlighting

While not a huge pro, this is still really nice for people who do a lot of CSS work.

4. Enforces consistent styling

If we have a style for a specific type of component, styled-components makes it very easy (and preferred) to reuse that style in as many places as possible. This way, the styling throughout the app can stay very similar without rewriting a bunch of CSS.

Cons

1. Have to rewrite all CSS

While this can be done in increments, it's still a lot of work.

2. Probably won't solve everything

I'm guessing there's still places where we'll need to fallback to regular CSS.

@ericadamski
Copy link
Contributor

This video is a little long (it is actually a workshop) but the talk is given by https://github.com/mxstbr the co-creator of styled-components. Really useful in comparing the different types of styling solutions in react (not focused on react-native but IMO still applies here). The questions at the end are also very good in bring some reasons why CSS styling is good, but how styled-components either has the same features or does it better.

https://www.youtube.com/watch?v=ftSzsghg2io

This video convinced my team to start using styled-components in our application and it has made our development and testing much more declarative (IMO)

@machour
Copy link
Member

machour commented Oct 16, 2017

I don't know if we should be using styled-components or not (I didn't try them yet to see real-life benefits), but I know something for sure: we need to rethink how our global design is laid out.

Currently, every component/screen define its own styles, and it can sometimes leads to inconsistencies (different margins/paddings/sizes). Button were re-designed from scratch every time they were used before rolling up our own <Button /> component and using it everywhere.

Also, with our current implementation, introducing themes (#59) is not doable in a maintainable way.

@alejandronanez
Copy link
Member Author

Thanks for your feedback guys!

I think styled-components will be a good upgrade to our current stack if we think about the future, what @machour mentioned about #59 will be quite complicated to achieve using what we have right now.

Also, I think that by using styled-components we will make our codebase a little more easy to understand to newcomers or people with only css knowledge.
Since migrating everything to styled-components in 1 single PR will be huge, we can start migrating slowly and create good for beginners issues so we attract new people to the repo.

Thoughts?

@andrewda
Copy link
Member

@alejandronanez Sounds good! I totally agree with a slow approach on this, not one huge PR.

@machour
Copy link
Member

machour commented Oct 17, 2017

@alejandronanez sounds good to me too.

I'm just wondering if we will be separating styles from components (i.e have a styles/ directory with all styles and requiring the needed ones from components)

@housseindjirdeh
Copy link
Member

So I've always for some reason thought that because React Native already uses JS for locally scoped component styles as opposed to React for the web, styled-components wouldn't really be beneficial.

Reading this article and the video you linked @ericadamski - I think I'm sold 👍

@alejandronanez the plan you laid out sounds perfect. Incrementally moving forward updating all of our styles slowly will be nice. Moreover, definitely don't mind including a mention in CONTRIBUTING.md to note future styles should leverage styled-components wherever possible if everybody thinks thats a good idea

@housseindjirdeh
Copy link
Member

housseindjirdeh commented Oct 17, 2017

@machour that's a good point. I'm in favour with keeping it within the same file personally to keep the small-components and locally-styled focus the way it currently is.

@ericadamski
Copy link
Contributor

@housseindjirdeh another solution we employ is to keep small locally-styled components in a file called <main-component>.styled.js to show the relation to the main component but keep the bulk of boilerplate and non-logic code outside the main jsx file. Just a potential solution 😄

@housseindjirdeh
Copy link
Member

housseindjirdeh commented Oct 17, 2017

@ericadamski Thank you, I literally just finished having a chat with someone in work about how to structure things 😂

I'm sorry, I spoke too soon there without being more clear. Keeping things within the same file seems easier for now because that's how I've sort of built each of the components/screen. I agree you're on to something better @ericadamski. Ideally I think having a folder structure for every component might be something we need to move towards. For example:

- Button/
  - button.component.js
  - button.styled.js

And our container components (our "screens")

- Notifications/
  - notifications.component.js
  - notifications.styled.js
  - notifications.container.js
  - notifications.action.js
  - notifications.reducer.js
  - notifications.type.js

Thinking in this pattern we can use recompose in our container files to make things more streamlined. Now I know I'm going way overboard for this ticket but with that being said, a good starting point would be at least making our components folder driven and keeping our styled files separate. What do you think @ericadamski?

Let me know what everyone thinks as well 🙌

@alejandronanez
Copy link
Member Author

Hey @gitpoint/maintainers I'm glad you like the idea of using styled-components let me add a PR with the dependency and create an initial PR for a random component, then we can start creating more issues for beginners.

About the implementation details, it looks a little weird to me to have the style separated from the main component file (since we're not reusing those styles I think we don't need to move them to another file).

@chinesedfan
Copy link
Member

chinesedfan commented Oct 18, 2017

@alejandronanez I think it is a tradeoff. If styles are not reusable and not very long, then we can keep it in the same file. Otherwise, we need separate it out.

@alejandronanez
Copy link
Member Author

There you go. First styled components PR #495

@ericadamski
Copy link
Contributor

@housseindjirdeh I think that is a great idea! Scoping components and their associated parts (styled, actions, reducers, selectors) helps (IMO) large projects scale.

@alejandronanez awesome PR! 🦄

@housseindjirdeh
Copy link
Member

Nice thank you folks <3

Okay, let's keep things in the same file for the meantime as that's how things currently are. Opened a separate issue to really start exploring a modular folder structure #506 🕺

@SammyIsra
Copy link
Member

Quick question about styled components. From what I've seen, you declare Components with its own, encapsulated style. This seems fantastic, but how would we deal with styles and themes across the app?

For example, if we want all titles and headers to have the same font and color and nothing else, we could have an easy CSS rule for, say, className="titleStyle". And add it to those titles and headers. How could we do something similar with styled components? Would we declare headers somewhere else and import them where we need them?

Same question would go for CSS variables and the such, how do we address those?

@ericadamski
Copy link
Contributor

ericadamski commented Oct 19, 2017

@SammyIsra the idea behind component architecture and therefore styled components is that creating many small isolated chunks of a system (in the case of styled a frontend UI) and reusing the small isolated chunks allows consistency and simplicity when building large applications.

Your suggestion for a CSS rule like className="titleStyle" is actually a component style architecture on its own, its small, manageable isolated chunk of logic that can be easily reused. Styled components outlook/solution to this problem just allows that same pattern you are suggesting but in a declarative, more HTML familiar manor.

For example, consider a page header that contains a title, a sub-title and some container elements to allow some funky styling. Using class names as your component driver you will have something that looks like this: (NOTE: this is not react native but react, the same ideas apply to both)

const Header = () => {
    return (
          <div className="header">
               <div className="header--inner">
                    <div className="header--title--container">
                         <h1 className="header--title">Title</h1>
                         <span className="header--title__sub">Sub Title</span>
                    </div>
               </div>
          </div>
    )
}

If you contrast above with a styled components approach and notice the more declarative nature of the render method:

import { Container, Inner, Title, SubTitle } from 'styled.js';

const Header = () => {
     return (
          <Container>
               <Inner>
                    <Title>Title</Title>
                    <SubTitle>SubTitle</SubTitle>
               </Inner>
          </Container>
     )
}

Take <Title/> for example: we have just encapsulated the styles into a more human readable component that no one how uses it should actually care about the className or the styles that make it work, just as if you were to apply that className titleStyle

Beyond that, say you have to change the colour of the title if it is used as a primary title or a secondary title, this would involve 2 extra classes that override the parent style and now we have to consider collisions and silly CSS precedence. With styled components you can just pass props and use JS logic to apply it directly in the component, for example:

const Title = styled.h1`
     display: block;
     font-size: 2rem;

     color: ${props => {
          if (props.primary) return 'red';
          if (props.secondary) return 'blue';

           return 'white'; // default
     }};
`; // the simple styled component

// in use
<Title primary>Primary Title</Title>
<Title secondary> Secondary Title</Title>

Since this is all jS managing CSS variables becomes just managing JS variables like having a colours object that has the specified global colours:

const colors = {
    black: '#000',
};

// and we can use it in a styled component like
const Title = styled.h1`
    color: ${colors.black};
`;

The power of styled components matches if not exceeds that of plain CSS or even SASS/LESS and allows a more declarative style of writing react.

@SammyIsra
Copy link
Member

Fantastic explanation, thank you! @ericadamski

One last question: @andrewda mentioned "Syntax highlighting" as a pro for styled-components. All code snippets I've seen so far have no syntax highlight on the CSS section of the styles-components component declaration. How is that a pro for the library?

@ericadamski
Copy link
Contributor

ericadamski commented Oct 19, 2017

@SammyIsra @andrewda is talking about code editor syntax highlighting, currently the styling follows a JSON structure so it cannot syntax highlight for CSS, but styled components you write actual pure CSS in JS so there is CSS style code syntax highlighting.

It just makes writing the code a little more comfortable. :)

@alejandronanez
Copy link
Member Author

Since we all decided that this is the way to go, I'm going to lock down this issue. If you think it's still worth discussing anything else, feel free to unlock it.

@gitpoint gitpoint locked and limited conversation to collaborators Oct 20, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

8 participants