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

WIP: CSS in JS Experiment #2710

Closed
wants to merge 13 commits into from
Closed

WIP: CSS in JS Experiment #2710

wants to merge 13 commits into from

Conversation

levithomason
Copy link
Member

@levithomason levithomason commented Apr 7, 2018

UPDATE

⚠️ The original experiment in this PR was a catalyst for our v2, which is now underway. See the fork https://github.com/levithomason/stardust

Please move any relevant discussions to the fork.


ORIGINAL

Regarding #2708. This PR is experimenting with porting styles to CSS in JS.

See the src/elements/Label/README.md in this PR for instructions, findings, and the latest info.

@levithomason levithomason force-pushed the feat/fela-experiment branch from 2294ab3 to 429fd2e Compare April 7, 2018 20:10
@levithomason levithomason force-pushed the feat/fela-experiment branch from 429fd2e to 5e88003 Compare April 7, 2018 20:15
@brianespinosa brianespinosa self-requested a review April 9, 2018 14:38
@levithomason levithomason force-pushed the feat/fela-experiment branch 4 times, most recently from 09b2828 to 527503b Compare April 9, 2018 19:51
@levithomason levithomason force-pushed the feat/fela-experiment branch from 527503b to f6b10a0 Compare April 9, 2018 19:55
@tim-soft
Copy link

tim-soft commented Apr 9, 2018

Would this technique improve tree shaking performance?

@levithomason
Copy link
Member Author

In terms of CSS, yes. The styles would be included in JS so they would automatically benefit from tree shaking. We already support the module field which enables tree shaking for components.

@brianespinosa
Copy link
Member

@tim-soft you should take a look at Fela to see how it works. Basically it renders small class name, atomic styles and applies those styles everywhere. Out of the box, it's mathematically the smallest number of CSS rules you could have. Everything in your project that has a width: 100% rule would get the atomic class name assigned to it, which would be something like .ab {width: 100%}. Major reduction in styles because of this.

@levithomason
Copy link
Member Author

levithomason commented Apr 9, 2018

Quick note, I'm initially experimenting with the monolithic() renderer enhancer. Class names in this PR are currently as follows:

<Label content={23} />
<div class="ui-label">23</div>

See more on class names in the PR's readme here:
https://github.com/Semantic-Org/Semantic-UI-React/blob/feat/fela-experiment/src/elements/Label/README.md#classname

@brianespinosa
Copy link
Member

Someone (me) obviously did not read that yet. ;)

.replace(/;/g, '') // remove semis

return wrapLessVariablesFile(type, name, content)
}
Copy link
Member

Choose a reason for hiding this comment

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

⏫ This was the part I was wondering how we would handle. Love it.

I still have not had a chance to dig into this because of a release we need to ship this week... and I am heading out of town for a wedding on Thursday. I'll take a look on the plane though for sure.

Copy link
Member Author

Choose a reason for hiding this comment

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

It isn't pretty, but it actually is working quite nicely so far! I was testing this out on the Label component by manually pulling things over, now I'm automating that and testing against the List component. Next, I'll try to do this with the site variables and the entire LESS build.

Once we have this pulled over once, we won't have to do it again. I am quite certain that we'll outpace SUI core CSS/LESS in features and fixes due to the size and speed of our community. That means we won't constantly update from them anymore but move forward instead.

Copy link
Member

Choose a reason for hiding this comment

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

Once we have this pulled over once, we won't have to do it again.

I was actually wondering about your thoughts on that. I guess technically we could just reference the npm package for those definitions. We could have a script that would just convert them all again once SUI updates and we update our package.

I guess though that if we move forward at a faster pace we might potentially have significant conflicts to resolve.

@levithomason
Copy link
Member Author

Heads up, I'm experimenting with a script that ports the entire LESS framework to JS in one go. This is certainly doable for themes (variables) whereas the definition files will likely need migrating by hand as they have engineering impact concerns.

This branch will be quite large and quite a mess while I flesh this out...

@layershifter
Copy link
Member

@levithomason awesome work, wow 👍 BTW, you can babel generator to create JS files, take a look there.

@levithomason
Copy link
Member Author

levithomason commented Apr 17, 2018

@layershifter nice, I'll have a look. Some of the LESS is pretty complicated but is not too bad to regex over to JS given how simple and consistent it was written. This is another reason I favor simplicity, so much easier to work with in the long run!

I'm currently using a script in node src/styles/lessToJS.js which pulls the *.variables and *.less files in with gulp, then parses them. This makes copies of src/styles/less into src/styles/js. These are not 100% correct, but a great start.

I am then copying one component at a time from src/styles/js to the specific component directory. So, src/elements/list/ has ./listVariables.js and ./listRules.js. I then update the component to use the new createComponent() and consume these styles. That makes them render in the doc site.

Finally, I can step through the component's various types, content, variations, etc. and make decisions about how we'll convert the CSS to JS.

I'm currently focused on the List component (previously testing the Label). The basic idea is working well. Any input or suggestions you might have, I'm all game!

@levithomason
Copy link
Member Author

I noticed you used the css module against the semantic-ui-css package for the icons. I'm using the semantic-ui-less package as I also need to port the variables over. I struggled to find a great way to get the LESS AST. The LESS docs and API are not so robust for that.

@layershifter
Copy link
Member

layershifter commented Apr 17, 2018

I tried to start work with CSS-in-JS some time ago, too. I wrote a simple parser, but it is very confusing and dirty 🦀 I cut a small snippet from it:

import * as less from 'less'
import * as fs from 'fs'
import * as path from 'path'

const file = path.resolve(__dirname, '../node_modules/semantic-ui-less/definitions/collections/menu.less')

fs.readFile(file, 'utf8', function (err, contents) {
  let content = contents.toString();
  content = contents.replace('@import (multiple) \'../../theme.config\';', '')

  less.parse(content)
    .then(function (output) {
      console.log(output.rules[18].selectors[0].elements[0])
    })
    .catch(function (error) {
      console.log(error)
    });
});

Possible, it will be usefull for you. Should note, that LESS AST is quite confusing and is very bad documented 😞

I used AST Explorer for CSS and Babel tryouts, it's very usefull.

@levithomason
Copy link
Member Author

Thanks for the snippet. This is almost identical to my first attempt as well. I found the AST traversal to be brittled as some nodes contain values while others have very nested values. I was hoping to find a great LESS AST traversal lib, but realized I was writing my own. That is when I switched to basic regex.

What was your conclusion?

@layershifter
Copy link
Member

layershifter commented Apr 17, 2018

I was hoping to find a great LESS AST traversal lib, but realized I was writing my own.

Same thing. I started to write own traverser on TypeScript, there was ugly result 😸 I think that we really can go with regex for start.

I had many problems with nested structures, example:

    const { value: name } = _.head(node.name)
    const { value: rawValue } = node.value
    const value: string = _.isArray(rawValue) ? _.head(_.head(rawValue).value).name : rawValue

:pain:

@levithomason
Copy link
Member Author

levithomason commented Apr 20, 2018

I've pulled some of this work over to a project at my day job. I'll be going back and forth between the two as I move forward. Still making progress.

@brianespinosa
Copy link
Member

Once we have a more solid foundation and pattern to follow for the way forward I can probably pull some of this in for work at my day job as well in the coming months.

@adamchenwei
Copy link

adamchenwei commented Apr 24, 2018

sorry, so many redirects to this thread, what about styled-components approach? why semantic ui dropped it? Just curious

@levithomason
Copy link
Member Author

what about styled-components approach?

#2708 (comment)

why semantic ui dropped it?

Semantic UI has never used styled-components.

@levithomason
Copy link
Member Author

I have been doing a lot of work on the side for this. I'm going to start pulling this work into a v2 branch.

What about v1?! The current state of the repo is v1. It is feature complete and needs only minor API updates. It will also be tied to SUI CSS forever. Once we finish the v1 milestones I'll release v1 and start working almost exclusively on v2. v1 will still be supported for bug fixes and minor features and any applicable v1 updates will get ported to v2.

Over the next week, I'm going to formalize the v2 Manifesto with @davezuko. This will very much be a review of what we love about SUIR v1, what we want to change, and what lessons we can take from other great libs like Ant Design, Material UI, and others. We will cover many aspects there and welcome much feedback.

Stay tuned!

@HerbCaudill
Copy link

@levithomason - Looking forward to the manifesto. I need this for a project, and I'm more than happy to help with the conversion.

@levithomason
Copy link
Member Author

Update:

The v2 manifesto is coming together well. I've ported all of the features in this experiment to the v2 branch, including many more updates and improvements.

Manifesto

See the PR #2802 or rendered file, MANIFESTO.md.

Knobs

Knobs allow you to add controls to your examples. The code for the knobs does not clutter the example code, but it passed in via props.

http://g.recordit.co/zUHldiRc4O.gif

Theme Variables

Component specific theme variables are now picked up from context for each example. A basic form is provided to edit them, for now.

As we scale up the number of theme variables available for components, we'll likely move this control to a full-height side rail, perhaps replacing the current right navigation menu.

http://g.recordit.co/Wgug7gILeH.gif

v2 Components

So far, we've scaffolded out these v2 components:

  • Provider (provides renderer and theme)
  • Layout (new low level control for handling layouts, more info later)
  • Image
  • List

These components still need cleanup and tests. Our tests also need to be updated to handle the new component patterns.

Fork

Once we merge the v2 List, #2803, and additional Layout component examples, #2805, we'll fork this repo to continue efforts. The current v2 branch will become the master branch on the fork.

At this point, we'll be adding several more collaborators and proposals. These proposals will cover the core concepts of v2. They will also formalize the contract by which v2 components operate.

These v2 contracts will be put into a core library that powers SUIR v2. That core library will be called stardust. Stardust will essentially be a utility package for creating a CSS in JS React component library.

...stay tuned

@brianespinosa
Copy link
Member

@levithomason regarding Knobs, since those seem to be mostly OOTB html form controls with no styling, are you opposed to PRs giving those some polish? If so, let me know. Something like that MIGHT be better to do after the official stardust change and fork, but I could start poking at it now.

Additionally, I can probably throw together a quick place for theme level variables to live in a drawer that we can start utilizing now. Again, something that might be better for after the fork.

@levithomason
Copy link
Member Author

@brianespinosa the fork is live now, https://github.com/levithomason/Semantic-UI-React. I've added you as a collaborator.

@levithomason regarding Knobs, since those seem to be mostly OOTB html form controls with no styling, are you opposed to PRs giving those some polish?

Some styling would be cool. I'd also like to be able to share these knobs with v1, so making the styles as a simple CSS file would be ideal.

I did want to avoid using SUIR components in the knobs area for two reasons. One, it makes it more clear which controls are part of the Knobs area and which are part of the example. Two, we don't have a control for the range slider, so just keeping things consistent.

@levithomason
Copy link
Member Author

⚠️ The original experiment in this PR was a catalyst for our v2, which is now underway. See the fork https://github.com/levithomason/stardust

Please move any relevant discussions to the fork.

@Semantic-Org Semantic-Org locked as resolved and limited conversation to collaborators Jun 1, 2018
@levithomason levithomason deleted the feat/fela-experiment branch June 1, 2018 18:07
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants