Theme-ability progress #3572

ndelangen · 2018-05-11T15:44:51Z

Issue: -we want storybook to be theme-able / customisable-

What I did

I changed more components to use emotion for styling, I also modified some components to prepare for customisation/theming ability.

It also includes a new mobile layout.

So I migrated everything over to use emotion.. surprise. It's basically because of this:
paypal/glamorous#419

Also I was running into a strange snapshot mismatch issue. Hopefully emotion doesn't have this issue. 🤞

How to test

Run the examples, see chromatic diffs

…orous && REFACTOR mobile layout

…rous-displayname babel-plugin

…view is default panel open

…DD shortcutshelp story

…ug in jest)

…' message appears

tmeasday · 2018-05-18T02:33:10Z

Pity there isn't a good way to look at all the changed stories at once @ndelangen 🤔

Code-wise, looks great. Did the font color change slightly?

ndelangen · 2018-05-18T11:29:26Z

The TeamCity official example is a pretty good indicator.

Indeed the main colour changed a bit, I'll work on a follow-up PR that consolidates the UI, and implements theming in more places.

shilman

Looks great! Minor comments inline 👍

shilman · 2018-05-19T00:52:57Z

addons/a11y/src/components/Tabs.js

+const Container = styled('div')({
+  width: '100%',
+  position: 'relative',
+});


...baseFonts?

No I want to cascade the fonts and colours down as much as possible to make themeing easier.

This is an extremely good idea @ndelangen

shilman · 2018-05-19T01:05:18Z

addons/knobs/src/react/WrapStory.js

+
+import PropTypes from 'prop-types';
+import React from 'react';
+import { polyfill } from 'react-lifecycles-compat';


What does this have to do with theming? Can we move to another PR?

shilman · 2018-05-19T01:08:14Z

addons/viewport/src/manager/components/Panel.js

@@ -170,11 +170,6 @@ export class Panel extends Component {
      this.iframe.style.height = viewport.styles.width;
      this.iframe.style.width = viewport.styles.height;
    }
-
-    // Always make parent's height equals iframe
-    if (this.iframe.parentElement) {


Why delete this???

Because I changed the way the overflow works for the preview

shilman · 2018-05-19T01:12:51Z

lib/components/README.md

@@ -11,4 +11,4 @@

 Storybook Components is a React UI components collection used by the UI of Storybook and Addons.

-All components use [`glamorous`](https://glamorous.rocks) for styling.
+All components use [`glamorous`](https://styled.rocks) for styling.


https://emotion.sh/

Keraito

Refactor wise looks great as far as I could tell! Just some small remarks or questions.

Keraito · 2018-05-19T15:38:41Z

addons/a11y/src/components/Report/Elements.js

-    marginBottom: '4px',
-  },
-};
+const Item = styled('li')({


This is entirely based on code only: Is there a specific reason why the li element is styled instead of the ol? The previous implementation had the styles.element injected into both of them which looked redundant, so removing one of them is great. But functionality wise it doesn't seem like there's a difference between putting it on the li and ol. Readability wise this looks better, but is there any possible downside (performance/verbose output for DOM of the user) for not doing so or any explicit reason to do so? Mostly questions out of curiosity!

Because the li actually has user-agent styling applied to it, the ol doesn't as much.

Keraito · 2018-05-19T16:08:10Z

lib/components/src/layout/desktop.js

+};
+
+class Layout extends Component {
+  constructor(props) {


A decent amount of other components in this PR have their constructor and method bindings replaced by class properties and fields, while a lot have not. Would it be wise to choose 1 and stick with it throughout the project?

I tend to write components using modules and module scope function and less with component instance function. That's just my coding style. (composition over inheritance)

Though I do believe that in a perfect world a codebase would be perfectly uniform, in reality code is messy for lots of reasons. I'd rather have people contribute and do a bit of gradual cleanup afterwards.

I personally think that code uniformity is highly overrated. It's very easy to be opinionated about and loose valuable time controlling petty details.

So to answer your question:
Yes it would be wise, but it's not something I'm willing to enforce, because there is a trade-off with getting new developers on board whose opinion and expertise is going to be different. I just apply the boy-scout rules: leave the campsite better then it was.

Keraito · 2018-05-19T16:12:42Z

examples/html-kitchen-sink/package.json

  "description": "",
+  "keywords": [],
+  "license": "ISC",


Is there a reason this is the only subproject with a ISC license? From what I could find, there doesn't seem to be a major difference with MIT, but just curious why one folder is differently licensed.

No there's no reason, the only reason this is in the PR, is because I auto-sort package.json files.

Please feel free to change it to MIT.

ndelangen added 23 commits May 10, 2018 12:53

MOVE & REFACTOR addon-panels to lib/component & glamorous

e51b713

MOVE & REFACTOR layout component to lib/components & style using glam…

16c257e

…orous && REFACTOR mobile layout

MOVE header to lib/component & REFACTOR to use glamorous && ADD glamo…

0fa6a51

…rous-displayname babel-plugin

UPGRADE glamorous

4aa547b

IMPROVE styling of storysource

67935e7

IMRPOVE scrolling behavior

3b461e2

IMPROVE scrolling of storysource

5f131ca

FIX a minor alignment issue

d7fe462

REFACTOR StoriesPanel to use glamorous && CHANGE mobile layout so pre…

f417484

…view is default panel open

REFACTOR searchbox & shortcutshelp & treeheader to use glamorous && A…

22f412e

…DD shortcutshelp story

REMOVE babel-plugin-glamorous-displayname from test and prod (has a b…

a5f3cd5

…ug in jest)

FIX snapshots not updating even when passing in --update

0ee32b3

CHANGE ui for easier themeing

c7b8d59

FIX proptype warning

7dd7426

REFACTOR preview wrapper to use glamorous

ecdb576

FIX tests && FIX snapshots

fe39ac8

CLEANUP previous mobile nav

c710cb9

REFACTOR mobile layout to use glamorous

9d7d724

FIX typo

21bc78f

ADD a placeholder component & use it at places where a 'this is empty…

fb3c3fc

…' message appears

CHANGE styling of shortcuthelp component

88fedce

FIX snapshots

d0426ba

ADD 'jest-glamor-react' to get more meaningful snapshots

e0e77b6

ndelangen added ui maintenance User-facing maintenance tasks components labels May 11, 2018

ndelangen self-assigned this May 11, 2018

ndelangen requested review from tmeasday, Hypnosphi and igor-dv May 11, 2018 15:44

ndelangen requested review from alexandrebodin, leonrodenburg and saponifi3d as code owners May 16, 2018 21:55

ndelangen changed the title ~~[WIP] Theme-ability progress~~ Theme-ability progress May 16, 2018

ndelangen added 4 commits May 17, 2018 00:50

IMPROVE desktop layout

e86936c

FIX double-escaping

5b10805

CHANGE package.json files so they are sorted nicely

3925f5a

REFACTOR to use emotion for Logger.js

d69c0d8

ndelangen requested review from aaronmcadam, Gongreg, kazupon, thani-sh, naipath and rmevans9 as code owners May 16, 2018 23:09

ndelangen added 4 commits May 17, 2018 01:09

FIX snapshots

903d6b1

REFACTOR addon viewport story to use emotion for styling

d33d53b

Merge branch 'master' into feature/more-components

aec2521

FIX snapshots

7d971b8

tmeasday approved these changes May 18, 2018

View reviewed changes

Merge branch 'master' into feature/more-components

43279a3

shilman requested changes May 19, 2018

View reviewed changes

FIX review comments

552a01c

Keraito reviewed May 19, 2018

View reviewed changes

Hypnosphi approved these changes May 19, 2018

View reviewed changes

ndelangen merged commit c821484 into master May 19, 2018

ndelangen deleted the feature/more-components branch May 19, 2018 22:39

Keraito mentioned this pull request May 20, 2018

Make all licenses consistent to MIT #3611

Merged

leandrooriente mentioned this pull request May 30, 2018

glamorous package is not maintained anymore, time to move to emotion #3689

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Theme-ability progress #3572

Theme-ability progress #3572

ndelangen commented May 11, 2018 •

edited

Loading

tmeasday commented May 18, 2018

ndelangen commented May 18, 2018

shilman left a comment

shilman May 19, 2018

ndelangen May 19, 2018

Hypnosphi May 19, 2018

shilman May 19, 2018

shilman May 19, 2018

ndelangen May 19, 2018

shilman May 19, 2018

Keraito left a comment

Keraito May 19, 2018

ndelangen May 19, 2018

Keraito May 19, 2018

ndelangen May 19, 2018

Keraito May 19, 2018 •

edited

Loading

ndelangen May 19, 2018

Theme-ability progress #3572

Theme-ability progress #3572

Conversation

ndelangen commented May 11, 2018 • edited Loading

What I did

How to test

tmeasday commented May 18, 2018

ndelangen commented May 18, 2018

shilman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Keraito left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Keraito May 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ndelangen commented May 11, 2018 •

edited

Loading

Keraito May 19, 2018 •

edited

Loading