twlevelup / watch_edition_react

A smartwatch simulator, built in ReactJS
MIT License
3 stars 2 forks source link

RFC: Styling of components #2

Closed watsonarw closed 7 years ago

watsonarw commented 7 years ago

How do we want to style components?

Options (from the top of my head) include:

1) "Old school" separate CSS file, JS has no context of CSS, and we include CSS at the top level 2) Use sass and separate SCSS files that get munged together somehow (either with webpack, or something else) 3) Single CSS file per component, included by the component JS (e.g. import './MyComponent.css') with a naming convention (e.g. BEM) for ensuring component styling doesn't clash. Use webpack to bundle everything together. 4) Similar to 3), but use CSS modules to generate unique classNames instead of having a naming convention. 5) Styled components, CSS defined inside the component JS file. 6) Something else

raytung commented 7 years ago

There's too much magic happening in 3. I remember how confused I was when I first got into the webpack ecosystem. If we choose to go ahead with import './my.css' I can imagine students asking about it and how much more we have to explain what's going on.

alex-mitchem commented 7 years ago

I feel like "old school" will be more familiar for most students.

sinan-aumarah commented 7 years ago

Hi Andrew,

The current project has each component completely isolated from the rest. Each component has its own scss file separate and injected using webpack's cssloader plugin/s. This is similar to what you mentioned in option3 but using scss. Please have a look at the project let me know what you think.

I'm a big fan of pure component design ( coming from angularjs).

Maybe we can organize a quick meeting at the office to show you guys what we already have and the direction moving forward. Any feedback back will be greatly appreciated.

Cheers,

sinan-aumarah commented 7 years ago

Sorry guys I just read your messages. I get your concern and I kind of agree about the confusion it might cause to some students. Sorry I haven't done the levelup exercise before and I thought we have to perfect it so and make it easier for them. The way I was thinking is that, you add a scss file and u inject it to your component and let the magic happens. I guess the real question is how simple do we want this exercise to be? I now feel like what we have in the new reactjs repo is an overkill for you just described.

Is there a possibility we can organise a meeting for like an hour at the office if you have time?

alex-mitchem commented 7 years ago

My main concern is managing how many new things the students will have to learn. If we aren't careful, LevelUp might turn into course on a bunch of different Javascript libraries/frameworks (I'm exaggerating of course, but you get my point).

raytung commented 7 years ago

Yeah I agree with Alex. I would like to have the project as explicit as possible so we can focus on teaching development practices rather than JS libraries.

sinan-aumarah commented 7 years ago

I get you guys. Thanks for that. I'll think in that case I'll move the SCSS to the root/asset/ level and change it.

watsonarw commented 7 years ago

I'd like to argue against SCSS, for one it's yet another thing that we'll have to explain, and secondly, it's another build step that slows things down. Thirdly, node-sass is a rather large, native binary that is prone to cross platform issues.

As much as it breaks my heart, perhaps a single, large, "old school" css file is the simplest way to handle it...

raytung commented 7 years ago

Ditto Andrew.

I just did ./go install and it started compiling some c++ code for node-sass. Luckily I have latest XCode command line dependencies, or else I'll have to update 2GB of tools before I can compile node-sass. The Mac developer ecosystem is pretty weird.

adamhope commented 7 years ago

Here's my 2c.

Students need to CSS to do two things:

  1. Add styling that is applied to ALL pages in the app.
  2. Add styling that is unique to a particular page in the app.

I'm totally against BEM etc. Students should not have to learn a variation of CSS, they should be able to write more or less pure CSS.

The ways they should be able to target pages is either by using a generic classname that appears on all pages, or an ID that is (should be) unique to a specific page.

I'm personally in favour of one CSS file per page BUT only if they don't have it's easy to include, as in ideally they wouldn't have to manually import it.

In writing this I'm starting to think for milestone one a single CSS file may be the best route.

SASS was good because you could write pure CSS and take advantage of the benefits if you knew how to to use i.e. we could. But if installing SASS is one of the problems I'm all in favour of ditching it.

alex-mitchem commented 7 years ago

Another plus for the big css is we can probably copy a lot of it from the old build.

yundifu commented 7 years ago

Hi Guys, what is the final decision here? Putting all styling in one css file or one css for page?

watsonarw commented 7 years ago

Looking at this from the perspective of making a new page (which is the thing the students will be doing the most)

CSS File per page

/* src/app/pages/MyPage/MyPage.jsx */

import React from 'react';
import BasePage from '~/framework/Pages/BasePage';

import './MyPage.css'

const MyPage = (props) => (
  <BasePage><h1 className="hello-world-heading">Hello World</h1></BasePage>
);

export default MyPage;
/* src/app/pages/MyPage/MyPage.css */
.hello-world-heading {
  color: #fabbac;
  font-size: 32px;
}
/* src/app/App.js */

import React from 'react';
import { HashRouter, Route } from 'react-router-dom';

const App = (props) => (
  <HashRouter>
    <div>
      <Route path="/my-page" component={MyPage}/>
    </div>
  </HashRouter>
)

export default App;

Global CSS file

/* src/app/pages/MyPage/MyPage.jsx */

import React from 'react';
import BasePage from '~/framework/Pages/BasePage';

const MyPage = (props) => (
  <BasePage>
    <h1 className="my-page-heading">Hello World</h1>
    <p className="my-page-content">This is a page with some stuff on it!!</p>
  </BasePage>
);

export default MyPage;
/* src/app/App.js */

import React from 'react';
import { HashRouter, Route } from 'react-router-dom';

import './App.css';
const App = (props) => (
  <HashRouter>
    <div>
      <Route path="/my-page" component={MyPage}/>
    </div>
  </HashRouter>
)

export default App;
/* src/app/App.css */

.other-page-heading {
  color: #fabbac;
  font-size: 32px;
}

.my-page-heading {
  color: #fabbac;
  font-size: 32px;
}

.probably-some-other-page-stuff {
  color: lime;
  border: 1px solid fuchsia;
}

.my-page-content {
  color: #fabbac;
  font-size: 12px;
  line-height: 16px;
}

After looking at that, I first thought one file per page, and tell them to import the CSS at the top of the file. But what about global styles (e.g. fonts, box-sizing, etc), and styles that are common between pages (e.g. headings)?

I'm kind of leaning towards a single App.css file, as I suspect that's where they'll just dump css anyway (if it exists for global/common styles), and it enforces us teaching them DRY.

Also, because we're not using a css naming convention or css modules, it will help minimise naming conflicts if all the CSS is in one place.

adamhope commented 7 years ago

In an ideal scenario I'd still vote for a solution that allows us to have a single global stylesheet plus one per watch page. The ones for each watch page should be written in such a way that all CSS is scoped to that specific page.

sinan-aumarah commented 7 years ago

Hi all,

We just removed all scss modules and changed all scss files to normal css. I believe that's what everyone has agreed on https://github.com/twlevelup/watch_edition_react/pull/20

The next decision whether to move to one file or not seems to be still in the air. Do you wanna vote?