Implement injectStyled by lttb · Pull Request #6 · cssinjs/styled-jss

lttb · 2017-04-22T16:16:30Z

Implement injectStyled from #4

kof · 2017-04-23T19:48:28Z

+  const {sheets} = styled
+  const {staticSheet, dynamicSheet} = sheets
+
+  const classNames = new Set([


Can we avoid Set? It needs a polyfil.

yeah, we can

kof · 2017-04-23T19:49:15Z

+      [name]: composeClasses(staticSheet.classes[name], dynamicSheet.classes[name]),
+    }), {})
+
+  return (...props: any) => createElement(InnerComponent, {sheets, classes, ...props})


props is always an object, no?

right, will fix it

I would probably not inject sheets at all. What use cases are there anyways? Also in react-jss I inject sheet which refs to dynamicSheet if there is one and to static if there is no. Actually I did it because it was already there for the case people still using props.sheet. From my experience there should be no need to use sheet object in react.

agree, I made it in relation to injectSheet, but it really doesn't look necessary

If current sheet prop passing in react-jss makes troubles, we might want to remove it from there also.

kof · 2017-04-23T20:06:06Z

+import composeClasses from './utils/compose-classes'
+import type {styledType} from './types'
+
+const injectStyled = (styled: styledType) => (InnerComponent: ReactClass<any>) => {


Where does ReactClass come from?

This is native flow type

kof · 2017-04-23T20:07:05Z

  jest: true
+
+globals:
+  ReactClass: true


Why is this needed, where do we use such a global?

eslint consider flow types and it works with rules like 'no-undef`, but it dosnt about this flow type. so we need to add it as global.

but maybe there is better solution with some plugin etc., I'll take a look

kof · 2017-04-23T20:07:31Z

-const createStyled = (jss?: Function = jssDefault) => (baseStyles: Object = {}) => {
-  let sheet
-  let dynamicSheet
+const createStyled = (jss?: Function = jssDefault) => (baseStyles: Object = {}): styledType => {


styledType should be capitelized, no?

Also lets use a type JssStyles = Object and type JssStyle = Object and put it into types, once we have a fully typed styles object for jss we can easily replace Object with the new one in types.

kof · 2017-04-23T20:11:38Z

+}
+export type StyledElementAttrsType = {tag: string, styles: Object}
+export type StyledElementType = Function & StyledElementAttrsType
+export type tagOrStyledElementTypeype = string | StyledElementType


should be capitalized

kof · 2017-04-23T20:12:21Z

+  const mountSheets = () => {
+    if (!sheets.staticSheet) {
+      sheets.staticSheet = jss.createStyleSheet(baseStyles, {
+        link: true,


why do you need link: true for static sheet?

kof · 2017-04-24T05:11:37Z

-    padding: 10
+    padding: 10,
+    '& + &': {
+      marginLeft; 10


kof · 2017-04-24T05:12:12Z

+  StyledType,
+  StyledElementAttrsType,
+  StyledElementType,
+  TagOrStyledElementTypeype,


kof · 2017-04-24T05:18:53Z

-
-      static styles = elementStyles
+      static tag: string = tag
+      static styles: ComponentStylesType = elementStyles


just realized: elementStyles should be elementStyle, ownStyles should be ownStyle

right, thanks

kof · 2017-04-24T05:25:29Z

+    tagOrStyledElement: TagOrStyledElementTypeype,
+    ownStyles: ComponentStylesType
  ): StyledElementType => {
    const {tag, styles}: StyledElementAttrsType = typeof tagOrStyledElement === 'string'


Lets use tagName instead of tag everywhere, its more correct and aligned with element.tagName

kof · 2017-04-24T05:26:07Z

@@ -32,9 +51,8 @@ const createStyled = (jss?: Function = jssDefault) => (baseStyles: Object = {})
    const staticTag = `${tag}-${++counter}`


I would call it considered previous comment staticTagName

kof · 2017-04-24T05:28:43Z

+          sheets.staticSheet.addRule(staticTag, elementStyles)
        }

-        if (dynamicStyles && !dynamicSheet.getRule(this.tagScoped)) {


Instead of tagScoped: tagId

maybe dynamicTagName?

kof · 2017-04-24T05:46:57Z

  ): StyledElementType => {
-    const {tag, styles}: StyledElementAttrsType = typeof tagOrStyledElement === 'string'
-      ? {tag: tagOrStyledElement, styles: {}}
+    const {tagName, style}: StyledElementAttrsType = typeof tagOrStyledElement === 'string'


tagOrStyledElement -> tagNameOrStyledElement

kof · 2017-04-24T05:51:01Z

+  const sheets = {}
  let counter = 0

+  const getScopedTagName = (tagName: string) => `${tagName}-${++counter}`


we can actually move it to the utils and call it generateTagName, similar to jss https://github.com/cssinjs/jss/blob/master/src/utils/generateClassName.js

kof · 2017-04-24T05:57:45Z

-        if (dynamicStyles && !dynamicSheet.getRule(this.tagScoped)) {
-          dynamicSheet
+        if (dynamicStyle && !sheets.dynamicSheet.getRule(this.dynamicTagName)) {
+          sheets.dynamicSheet


just realized we don't need sheets object any more.

we need, because we need mutated sheets here too https://github.com/cssinjs/styled-jss/pull/6/files#diff-f336995dd331140b2022485b207c9fc1R10

if we just export them, with reassign we will lost links to new sheets

do we reassign them somewhere else than in mountSheets?? Should evtl mountSheets return them?

no, you are right, we can just return them.
will fix it

kof · 2017-04-24T06:00:26Z

      props: StyledElementPropsType

-      tagScoped = ''
+      dynamicTagName = ''


we could set dynamicTagName = generateTagName(tagName) here and remove constructor override, right?

nope, cause we can call Components with different styles

<Button color="pink" /> <Button color="green" />

and we need to have a scoped tagName for each instance

kof · 2017-04-24T06:01:55Z


      componentWillUnmount() {
-        dynamicSheet.deleteRule(this.tagScoped)
+        sheets.dynamicSheet.deleteRule(this.dynamicTagName)


why do we remove the rule, we could reuse that rule next time the component mounts, no?

because we generate new scoped name in constructor, so we can't get and old dynamicTagName for the pervious instance

why? the instance will be reused in react ....

right, but react will call constructor and we will lose it

well, don't redefine it in this case if it is already defined?

kof · 2017-04-24T06:18:22Z

@@ -0,0 +1 @@
+export default (...args: any) => args.filter(Boolean).join(' ')


just realized, why do we use sometimes camel case and sometimes dash separated file names? In JSS I stick to camel case.

lttb added the enhancement label Apr 22, 2017

lttb added 5 commits April 23, 2017 01:28

Add es0215 babel preset

43853de

Update yarn.lock

0ad79df

Implement injectStyled, resolve #4

3a90aa3

Fix styledType

253a136

Update README.md for injectStyled

b555ba6

lttb force-pushed the feature/injectStyled branch from e29a692 to b555ba6 Compare April 22, 2017 22:30

Merge branch 'master' into feature/injectStyled

e994113

kof reviewed Apr 23, 2017

View reviewed changes

Avoid set and remove sheets prop in injectStyled

3ff81d1

kof reviewed Apr 23, 2017

View reviewed changes

Update types, remove link for static sheet

fbbb43b

lttb force-pushed the feature/injectStyled branch from 719f977 to fbbb43b Compare April 23, 2017 22:39

kof reviewed Apr 24, 2017

View reviewed changes

Comment thread README.md

padding: 10

padding: 10,

'& + &': {

marginLeft; 10

Copy link
Copy Markdown

Member

kof Apr 24, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo

kof reviewed Apr 24, 2017

View reviewed changes

Fix typos and naming

caec53a

kof reviewed Apr 24, 2017

View reviewed changes

Rename TagOrStyledElement -> TagNameOrStyledElement

35eac8b

lttb force-pushed the feature/injectStyled branch from 3de5ee1 to 35eac8b Compare April 24, 2017 05:49

kof reviewed Apr 24, 2017

View reviewed changes

lttb added 2 commits April 24, 2017 09:31

Move generateTagName to utils, use camelCase for filename

3860127

Refactor createStyled, move styled function to the separate file

0d79dbb

lttb force-pushed the feature/injectStyled branch from aafd1d4 to 0d79dbb Compare April 24, 2017 07:00

lttb added 2 commits April 24, 2017 10:05

Add tests to pre-commit hook

b375d41

Fix elementStyle type

41a24aa

lttb force-pushed the feature/injectStyled branch from 8ed9688 to 41a24aa Compare April 24, 2017 07:06

Move createStyled to the separate file

8a0a61d

lttb force-pushed the feature/injectStyled branch from 364d001 to 8a0a61d Compare April 24, 2017 07:46

lttb added 2 commits April 24, 2017 10:50

Add babel-plugin-transform-flow-strip-types

8cf691f

Cache generated dynamicTagName

775dcbd

lttb merged commit ca8f9b2 into master Apr 24, 2017

lttb deleted the feature/injectStyled branch April 24, 2017 08:50

		@@ -32,9 +51,8 @@ const createStyled = (jss?: Function = jssDefault) => (baseStyles: Object = {})
		const staticTag = `${tag}-${++counter}`

		@@ -0,0 +1 @@
		export default (...args: any) => args.filter(Boolean).join(' ')

Conversation

lttb commented Apr 22, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lttb Apr 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kof Apr 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lttb Apr 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

lttb Apr 23, 2017 •

edited

Loading

kof Apr 23, 2017 •

edited

Loading

lttb Apr 24, 2017 •

edited

Loading

lttb Apr 24, 2017 •

edited

Loading