Cleanup styles

[bvaughn]

Replace current global styles with either CSS modules or a CSS-in-JS solution to make them more modular.

[cyan33]

Hi Brian, I saw your twitter and am really interested in helping improve this awesome app.

Also, since I haven't used "CSS modules or a CSS-in-JS solution" before, I guess I could also learn something from it. Could you plz gimme some guidance or material to get some idea from?

Much appreciated!

[bvaughn]

Hey @cyan33 😄 Welcome!

I don't have a lot of time to offer in-depth guidance at the moment, but I can point you at some examples from projects I've worked on:

• CSS modules example: React Virtualized demo site
• Glamor example: ReactJS.org
• Styled Components example: React Presents
• Inline styles example: React DevTools

I think for this project, if I end up picking up this task, I'd lean either towards Glamor or CSS modules because I'm familiar with both and have limited time.

However if I had more time, I'd also be curious in checking out Glamorous and Emotion. 😄

If you would like to help with this task, let me know and I'll mark it in progress. We can also chat a bit here or on Twitter about ideas (if you'd like).

[bvaughn]

By the way, I'm happy to hear that you're interested in helping! 😁 Thanks!

[cyan33]

@bvaughn Thanks for your elaboration! I've already got some idea from these useful material and am much willing to help out with this task.

I used to write CSS module quite often, which maps react component to its own CSS module. One thing I'm little bit confused is that is there any advantage of using Glamor over CSS modules, because I see the reactjs.org is using this approach and previously I thought mixing CSS with JS was not a good practice.

Thanks! 😄

[bvaughn]

There's probably lots of articles about the advantages/disadvantages of CSS-in-JS. 😄 Some of it comes down to subjective preference. Some of it is improved flexibility with regard to theming (something I don't currently intend to add to this app.) Search around. 😁

Sounds like you'd like to take a stab at this? So it's all yours. Let me know if you change your mind and I'll remove the in-progress label.

[MikeTheCanuck]

I’m just a random guy on the Internet and there are many highly skilled front end developers who’ve put in a lot of work to get to this cutting edge. I got the same lessons - separate your logic from your design - and I’m still not sure that this new CSS-in-JS approach solves more problems than it causes.

But it seems more appropriate to the times when you don’t have separate UX and engineering teams - when your front end dev is front-end dev+design, then there are clear advantages to the same person seeing the whole picture (even though it still doesn’t solve the problem of a consistent end-to-end design through a suite of software that should all present the same look and feel). Trade-offs.

[cyan33]

@bvaughn @MikeTheCanuck Thanks for your explanation. Really helpful to get me started. I'm taking this issue and am doing some research. Hopefully I'll submit a PR soon. 😃

[bvaughn]

I look forward to seeing it. But also, take your time. Enjoy the holidays. 😄

[cyan33]

@bvaughn Merry Christmas Brian, after some research I decided to go for the glamor approach (the idea of combining css and js is not that bad :) ) Here are some things that I'm not sure how it should actually work:

1. Using inline css is similar to local state in react. Usability wise, you have to “lift” the common style and pass down as props to its children components, which makes the parent component (App.js) not that compact. Also, lifting the styles makes it less modularized. Should I duplicate the style in each of the child components or lifting it to their common ancestors?

2. The syntax that reactjs.org uses is css={{ }} while what I used (according to the docs) is
   {…css({ })}. Does that matter?

Enjoy the holidays!

[bvaughn]

Using inline css is similar to local state in react. Usability wise, you have to “lift” the common style and pass down as props to its children components, which makes the parent component (App.js) not that compact.

I'm not sure I really understand this comment. Could you provide some minimal example? I don't think using Glamor should have much of an impact on App.js for example.

Should I duplicate the style in each of the child components or lifting it to their common ancestors?

Maybe create a few, reusable, styled components? Similar to the CSS classes but instead of eg .new-form-section-header it could be a component FormSectionHeader?

The syntax that reactjs.org uses is css={{ }} while what I used (according to the docs) is {…css({ })}. Does that matter?

I like the css={...} form better for readability, although it's not a huge deal. To use it, we'd just need to use Glamor's createElement decorator instead of the default React one.

https://github.com/threepointone/glamor/blob/master/docs/createElement.md

[cyan33]

I agree with the idea of creating a few styled components. An example could be like this: The EditExerciseForm, EditSleepForm, EditFoodForm, and EditSymptomForm share many similar styles. Instead of lifting all the shared styles to App.js and pass down to those components as props, I'm thinking that maybe we could create a styled HOC component like EditForm, which is similar like this:

function EditForm(WrappedComponent, styles) {
  return class StyledWrappedComponent {
    render() {
       return <WrappedComponent css={styles}>
    }
  }
}

In this way, a intermediate component is used to pass down the styles. I don't know if I'm getting it right. The downside is that the styles are passed down as a whole object, which is not very glamor flavor. 😢 Could you please share some of your thoughts?

[bvaughn]

I'm not sure I understand the HOC proposal, or this comment:

The downside is that the styles are passed down as a whole object, which is not very glamor flavor.

Here's more what I had in mind, eg the Header component from the reactjs.org repo:

const Header = ({children}: {children: Node}) => (
  <h1
    css={{
      color: colors.dark,
      marginRight: '5%',
      ...fonts.header, // fonts is a shared "theme" export
    }}>
    {children}
  </h1>
);

Also, as I mentioned above w.r.t fones, if there are shared styles, or things like colors/fonts, that are shared between many components- we could factor them out into a "theme" file, similar to how reactjs.org does.

[bvaughn]

I suggest you pick a single component (eg LoadingSpinner) or screen (eg the LoginRoute) and create a PR for it first. Then we can talk over the specifics and go from there.

[cyan33]

Here is a PR I submitted yesterday. And left some things for discussion. 😄 https://github.com/bvaughn/personal-logger/pull/22

Also, I think the "theme" idea is great, since most of the shared styles do with form and list.

[bvaughn]

I left a couple of comments inline that refer back to comments on this issue. It's late and I'm too tired to go into much depth at the moment unfortunately. Hopefully they're helpful?