-
Notifications
You must be signed in to change notification settings - Fork 81
Conversation
…c styleguide based on webpack2
Please ignore linting. I'll switch cf-ui to prettier soon, so I didn't care. |
This is ace, thanks for putting it up for review! I haven't had a chance to go over the It's worth noting that there's currently an internal project to create a styleguide, so I'd hold off creating anything too revolutionary for now. I'm all for refactoring into a styleguide app (in fact, that's exactly what I did when I created my dynamic styleguide proof-of-concept; making a webpack app in Also FWIW, as part of the dynamic styleguide I made (the one that gave you editable PropTypes via HTML inputs) I created a PropTypes webpack loader that used |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is awesome. I see some short hands used but I guess they are allowed since it looks like fela just uses the DOM's style properties. It's been a while since I've used them, it's a bit surprise to rediscover that some CSS properties don't support shorthands, but some do. Is this still a concern?
Just a few question, since we've moved all docs to go under /docs so Github Pages can serve them, this styleguide is intended to replace that at some point right? Also, now that webpack is in place, this means we can conceivably remove a hugh chunk of code from the gulp file that has to do with the examples, and pave the way to use webpack to compile all the components in the future right? Can you write down your plans on the Webpack ticket?
package.json
Outdated
}, | ||
"devDependencies": { | ||
"autoprefixer": "^6.3.6", | ||
"babel-cli": "^6.23.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you need this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch. I'll remove it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
@@ -0,0 +1,46 @@ | |||
const React = require('react'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why the trailing underscore on packages/cf-component-button/test/Button.js_
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
outdated nvm
@hturan It seems we are working on the same thing. Can you send me a link? To make it dynamic (with HMR) it needs to be a part of cf-ui repo. Only this way, you can have the best dev experience (no rebuilding, no refreshing). I think we should merge everything into the styleguide app. I'll bring it up to the current functionality. We can start adding new features later (live editing through HTML inputs would be really cool).
That's exactly my plan. @wyuenho You can use shorthand properties or longhand properties, but you can't combine them in the same rule. I think we should mostly avoid shorthand properties (especially for common properties as margin, border or padding). It adds some extra code but it will cut the size of CSS output in a long term since longhand properties are more unique and can't benefit from Atomic design that much. Also, it makes customization easier (if I used shorthand margin for the button component, its rule function would be a mess). I don't have a strong opinion on this yet. Let's see what our experience will be but not using shorthands definitely will not hurt. Ad webpack. Yes! Once we transition all components, we can remove gulp completely. Examples are handled by |
createComponent as createFelaComponent | ||
} from 'react-fela'; | ||
|
||
export const createComponent = (rule, type = 'div', passThroughProps = []) => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's call this node or element, type is overused
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's what fela is using now so I want to match their API/terminology if possible. Node / Element has meaning in DOM terminology. This actually accepts React components as well.
What's new
cf-style-container
, it is a wrapper for some fela APIs so we can keep the rest of components fela free/styleguide
dir/packages/cf-xxx-xxx/src/xxx.js
What's broken
What's next
cf-component-button
won't change much. I will have to do some updates so it supports our internal styles./styleguide
folder. So we can easily use it for private cf-ui as well. The same approach as with doca.