react-styleguidist icon indicating copy to clipboard operation
react-styleguidist copied to clipboard

WIP: Static rendering

Open okonet opened this issue 9 years ago • 48 comments

This PR implements static rendering of the styleguide when building for production.

🚧🚧🚧 Please don't merge yet. Still WIP!!! 🚧🚧🚧

okonet avatar Feb 18 '17 10:02 okonet

It looks like I was able to render everything besides the actual components (which is by design) statically using latest commit.

This includes:

  • TOC (Table of Contents) with proper slugs (this took some time)
  • Markdown documents
  • Code highlighted with highlight.js

We'll need to take some care of injecting CSS into head during the build time as well.

okonet avatar Feb 21 '17 16:02 okonet

@kof can you help me here with rendering styles statically? SG isn't using react-jss so no SheetsRegistry etc.

BTW @sapegin should we switch to react-jss? Can you elaborate in more details what's is missing from it to be able to use the "official" repository?

okonet avatar Feb 23 '17 11:02 okonet

Codecov Report

Merging #330 into master will decrease coverage by <.01%. The diff coverage is 97.95%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #330      +/-   ##
==========================================
- Coverage   94.77%   94.77%   -0.01%     
==========================================
  Files          75       78       +3     
  Lines         995     1014      +19     
  Branches      202      211       +9     
==========================================
+ Hits          943      961      +18     
- Misses         52       53       +1
Impacted Files Coverage Δ
scripts/schemas/config.js 96.96% <ø> (ø) :arrow_up:
scripts/make-webpack-config.js 100% <100%> (ø) :arrow_up:
loaders/utils/getStyleguide.js 100% <100%> (ø)
scripts/create-server.js 90.9% <100%> (ø) :arrow_up:
...sg-components/HtmlDocument/HtmlDocumentRenderer.js 100% <100%> (ø)
loaders/styleguide-loader.js 100% <100%> (ø) :arrow_up:
src/rsg-components/HtmlDocument/HtmlDocument.js 100% <100%> (ø)
...rc/rsg-components/ReactComponent/ReactComponent.js 100% <100%> (ø) :arrow_up:
src/rsg-components/Examples/Examples.js 83.33% <100%> (ø) :arrow_up:
src/utils/utils.js 96.92% <85.71%> (-1.44%) :arrow_down:
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 2f80d73...c82a157. Read the comment docs.

codecov-io avatar Feb 23 '17 11:02 codecov-io

@okonet I am not aware of decision details, it seems like react-jss would be a good fit here. In any case its easy. You can always create a SheetsRegistry and add the sheets to it and then get them all together as a CSS string.

kof avatar Feb 23 '17 11:02 kof

@kof can you commit the solution you're suggesting?

okonet avatar Feb 23 '17 12:02 okonet

@okonet @kof Basically these two lines. We need to merge styles with user’s styles and and also pass merged theme object. And strip Renderer for nicer class names.

I’m not sure it’s all possible to do with react-jss and what would be the best solution.

sapegin avatar Feb 23 '17 12:02 sapegin

Btw. WrappedComponent.name is not supported everywhere, haven't found caniuse data in this regard.

kof avatar Feb 23 '17 13:02 kof

react-jss uses options.meta property if you pass it, otherwise it falls back to a component name https://github.com/cssinjs/react-jss/blob/master/src/index.js#L36

kof avatar Feb 23 '17 13:02 kof

Why do you call them SomethingRenderer anyways and then replace the Renderer part in order to have nicer meta attributes on style tags? Why not to remove the Renderer thing from the name from the beginning? If it is not a container component, it is a renderer anyways, containers have no sheets.

kof avatar Feb 23 '17 13:02 kof

Btw. WrappedComponent.name is not supported everywhere, haven't found caniuse data in this regard.

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Function/name

Why do you call them SomethingRenderer anyways and then replace the Renderer part in order to have nicer meta attributes on style tags?

Because I have components without Renderer but they don’t have any markup.

sapegin avatar Feb 23 '17 13:02 sapegin

Btw. latest react-jss also passes classes object to the component additionally to the sheet.

kof avatar Feb 23 '17 13:02 kof

Migration should be trivial.

kof avatar Feb 23 '17 13:02 kof

@kof What about merging with custom styles and passing theme object?

sapegin avatar Feb 23 '17 13:02 sapegin

Because I have components without Renderer but they don’t have any markup.

So they are containers? Why is it important to identify a container by name? Why not call a container component SomethingContainer?

kof avatar Feb 23 '17 13:02 kof

Components without Renderer are “containers”:

function Button(props) {
  const stuff = do something with props
  return <ButtonRenderer {...stuff} />;
}

Renderers contains only HTML so users could easily change style guide rendering.

sapegin avatar Feb 23 '17 13:02 sapegin

And of course I don’t want to write ButtonContainer instead of Button everywhere ;-)

sapegin avatar Feb 23 '17 13:02 sapegin

@kof What about merging with custom styles and passing theme object?

Why are you using context to pass the config and take the user styles overrides if they are basically global? Why not just merge them in each styles definition?

kof avatar Feb 23 '17 13:02 kof

Because I don’t want users to do that when they override our components.

sapegin avatar Feb 23 '17 13:02 sapegin

@sapegin @kof let's move off-topic discussions to separate issues since they will be lost as soon we merge it.

okonet avatar Feb 23 '17 13:02 okonet

Ok should we make this whole styles discussion a separate thing? I see 2 possibilities: we add SheetsRegistry right now here. Otherwise I need to implement a ThemeProvider in react-jss. It is planned anyways. Right now the first one is faster.

kof avatar Feb 23 '17 13:02 kof

Yes, let's move the discussion and implement the fastest way and refactor later.

okonet avatar Feb 23 '17 13:02 okonet

Then let’s make fast now and then we’ll se if we can switch to react-jss :shipit:

sapegin avatar Feb 23 '17 13:02 sapegin

So should I do it or it is already clear enough?

kof avatar Feb 23 '17 13:02 kof

Please do this @kof! I've tons of other things to do now.

okonet avatar Feb 23 '17 13:02 okonet

Should it be on top of this branch? feat-static-rendering

kof avatar Feb 23 '17 14:02 kof

I'd say so since it belongs together.

okonet avatar Feb 23 '17 14:02 okonet

@kof Yup, you should have commit right now. Welcome to the club but don’t talk about it ;-)

sapegin avatar Feb 23 '17 14:02 sapegin

Ok I have added the code for sheets aggregation, but I couldn't test it, somebody need to show me how to kick this all off, or maybe it is right now in a broken state?

kof avatar Feb 23 '17 22:02 kof

@kof try yarn run build

okonet avatar Feb 24 '17 05:02 okonet

Ok, I have finished the SSR integration for JSS. I needed to do 2 suboptimal things, because of "react-html-document" component limitations.

  1. We need to get the styles used by the application before we render the document. That module assumes we don't. So I needed to render the app to string first and then pass it to HtmlDocument inside of a div with dangerouslySetInnerHTML.
  2. HtmlDocument abstraction doesn't give any option to define an attribute on server-side rendered style tag, in order to delete that tag once js kicks in I currently remove the first tag, but it will break if you add more style tags on the server and they will come first.

kof avatar Feb 24 '17 08:02 kof