-
Notifications
You must be signed in to change notification settings - Fork 37
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
Document <Spinner /> <Button /> <Icon /> #196
Conversation
2734cd8
to
0243891
Compare
You create a branch on the cozy repository, is that wanted? Why not a fork? |
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.
Too many unexplained changes on the configuration. Need more details to be approved.
docs/styleguide.config.js
Outdated
@@ -1,8 +1,11 @@ | |||
const path = require('path') | |||
|
|||
module.exports = { | |||
components: path.resolve(__dirname, '../react/{Button,Toggle}/*.{js,jsx}'), | |||
components: path.resolve(__dirname, '../react/{Spinner,Icon,Button,Toggle}/*.{js,jsx}'), |
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.
We shall not list components here, if we have 30 000 components, we could not list them anymore, could we?
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.
Exactly what I was talking about here : #180 (review)
I am doing the change
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
docs/webpack.config.js
Outdated
@@ -33,5 +34,14 @@ module.exports = { | |||
} | |||
] | |||
}, | |||
plugins: [new ExtractTextPlugin('[name].css')] | |||
plugins: [ | |||
new webpack.LoaderOptionsPlugin({ |
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.
What is the utility to use LoaderOptionsPlugin if we can set an option in the loader itself?
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.
Thanks, good catch, done !
docs/styleguide.config.js
Outdated
webpackConfig: require('./webpack.config.js'), | ||
serverPort: 6161, | ||
styleguideDir: path.resolve(__dirname, '../build/react') | ||
styleguideDir: path.resolve(__dirname, '../build/react'), | ||
require: [ |
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.
Cannot fund any documentation about require
in webpack.
What is used for?
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.
To require additional files : https://react-styleguidist.js.org/docs/configuration.html#require
react/Icon/index.jsx
Outdated
@@ -1,8 +1,10 @@ | |||
import React from 'react' | |||
import React, { PropTypes as Types } from '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.
https://facebook.github.io/react/docs/typechecking-with-proptypes.html
React.PropTypes has moved into a different package since React v15.5. Please use the prop-types library instead.
Unless it's added with preact-compat
. But I think to be more reliable, we should depend on the real proptypes module.
On the other hand, I cannot understand why you alias the import.
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.
On the other hand, I cannot understand why you alias the import.
Oh I'm sure you can :D : it is to type les characters. It is fairly common : https://github.com/search?utf8=%E2%9C%93&q=%22PropTypes+as+Types%22&type=Code
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.
What do you mean by to be more reliable ?
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.
preact-compat
is using the prop-types
module under the hood 👌 https://github.com/developit/preact-compat/blob/master/package.json#L87
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.
We are using preact
but to stay more reliable on react
, we also use preact-compat
.
For instance preact
let us declare the render
function with props
as parameters, react
doesn't. So we usen't.
The same goes to PropTypes
, since react
15.5, PropTypes
has moved to a different package.
If you look at the import
part of both below codesandbox, you'll understand:
- preact with preact-compat works with
import { PropTypes } from "preact";
(https://codesandbox.io/s/2okk0663rp) - react don't (https://codesandbox.io/s/kmj356yp15)
On the other hand, both works with import PropTypes from "prop-types"
.
In any event, both used the exact same release of prop-types
.
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
8673e6a
to
8aaece6
Compare
True it was a bad idea to push directly on the
Because it is tad more complex to manage the fork. Is it a problem if I push on cozy/cozy-ui ? |
8aaece6
to
c6afb24
Compare
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.
Some remarks.
react/Icon/Readme.md
Outdated
<div style={{ background: '#ccc'}}>{Object.keys(icons).map(icon => <span> | ||
{ icon } : <Icon icon={ icon } color={ colors[i++] } /><br/> | ||
</span>) }</div> | ||
``` |
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.
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.
paperplane icon missing
Bug from the icon, not from the docs
only the first 4 icons
Thanks, good catch ><
react/Icon/Readme.md
Outdated
<div style={{ background: '#ccc'}}>{Object.keys(icons).map(icon => <span> | ||
{ icon } : <Icon icon={ icon } color={ colors[i++] } /><br/> | ||
</span>) }</div> | ||
``` |
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.
const icons = require('../../src/icons');
const colors = ['red', 'blue', 'yellow', 'green']
let i = 0;
<div style={{ background: '#ccc'}}>
{Object.keys(icons).map(
icon => (
<span>{ icon } : <Icon icon={ icon } color={ colors[i++%colors.length] } /><br/></span>
)
)}
</div>
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 !
react/Icon/Readme.md
Outdated
<div style={{ background: '#ccc'}}>{Object.keys(icons).map(icon => <span> | ||
{ icon } : <Icon icon={ icon } color={ colors[i++] } /><br/> | ||
</span>) }</div> | ||
``` |
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.
Beware of the code sample, it lacks a closing parenthesis in the map function.
And format the code as it could be more readable.
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.
no parenthesis lacking here
|
||
import myIcon from 'my-icon.svg' | ||
|
||
<Icon icon={ myIcon } width='2rem' height='2rem' color='purple' /> |
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.
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.
What do you mean it ain't no purple ? Are you testing with an icon with a fill property ?
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.
Oh, I though the icon was the
I did not read the doc that explains we need to use svg-sprite-loader
.
react/Icon/index.jsx
Outdated
Icon.propTypes = { | ||
icon: React.PropTypes.string.isRequired | ||
icon: Types.oneOfType([Types.string, Types.object]).isRequired | ||
} |
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.
Add other props even if they are not required.
See https://github.com/yannickcr/eslint-plugin-react/blob/master/docs/rules/prop-types.md
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.
Thanks i added the properties. I presume you use this eslint configuration in your editor ? The lint command of cozy-ui only uses standard js, maybe you can add eslint ?
white: <Spinner color='white' /> | ||
red: <Spinner color='red' /> | ||
</div> | ||
``` |
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.
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.
It is a bug from the Spinner
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.
<div> | ||
<Spinner noMargin={ true }/> | ||
</div> | ||
``` |
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.
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.
Bug from the spinner
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.
xlarge: <Spinner size='xlarge' /><br/> | ||
xxlarge: <Spinner size='xxlarge' /> | ||
</div> | ||
``` |
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.
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.
Bug from the spinner
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.
|
||
```jsx static | ||
<Spinner loadingType='salut' /> | ||
``` |
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.
I don't want cozy-ui
to be dependent on <I18n />
.
But maybe, I am alone on this idea.
So please, consider this comment as a non blocking one, we should update later.
By the way, could you find a way to perform a running demo, maybe you can pass through a props like t={word => word}
.
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.
IMHO, this is not a problem of the docs.
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.
It allows to display a running demo.
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.
c6afb24
to
d13ab23
Compare
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.
Don't we need to fix <Spinner />
within the same Pull Request. I don't know.
Ok, for the other changes.
Even if we can improve the demo by fixing |
Thanks for the thorough review ! |
Visible on https://ptbrowne.github.io/cozy-ui/react/
Edited: changed the url to show the result on my repo.