TextualSummary: more stories, tests and exports. #25
Conversation
|
Checked commits martinpovolny/react-ui-components@67625ac~...f35d04f with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0 |
| image: PropTypes.string, | ||
| icon: PropTypes.string, | ||
| value: PropTypes.any, | ||
| label: PropTypes.any, |
There was a problem hiding this comment.
We should avoid using PropTypes.any. I know that sometimes it can be difficult, especially dorm some dynamic objects, but props like label should be defined properly.
Also, if you don't mark object attributes as required, prop validation will pass even for empty object. If there are some conditions, when should be attribute required, and whet it should be undefined, you can create custom prop validator. You can check this example: https://reactjs.org/docs/typechecking-with-proptypes.html
There was a problem hiding this comment.
Right. I did not describe all the data types in a lot of detail. I plan to improve that in follow up PRs. Together with the use of patternfly-react components where applicable.
| storiesOf('TextualSummary', module) | ||
| .add('GenericGroup', () => { | ||
| return ( | ||
| <GenericGroup items={genericGroupData.items} title={genericGroupData.title} onClick={e => null} /> |
There was a problem hiding this comment.
What is the purpose of onClick={e => null}? I would advise specifying this inside the GenericGroup component and simply don't pass any props.
You can use defaultProps of that component. Something like
GenericGroup.propTypes = {
onClick: PropTypes.func
...
}
GenericGroup.defaultProps = {
onClick: () => {}
}
There was a problem hiding this comment.
For a real use the onClick is mandatory. In the Storybook, I don't care.
I can replace it with some alert("You clicked!") if you want. But it looks silly.
No I don't want to have a default value. For a real use there needs to be a handler.
There I have a default on-click handler. BUT: that behavior is manageiq-ui-classic specific internal. For that reason it's there and not here.
Does that address your concerns?
There was a problem hiding this comment.
Ahh ok i remember now. I thought that there is some rule. Never mind then.
There was a problem hiding this comment.
I don't like e => null as well, since it does not give any information to user of such storybook. Can you please use https://github.com/storybooks/storybook/tree/master/addons/actions so the action is logged at least into storybook action list?
| <MultilinkTable | ||
| title={multilinkTableData.title} | ||
| items={multilinkTableData.items} | ||
| onClick={e => null} |
There was a problem hiding this comment.
again e => null, not a fan
There was a problem hiding this comment.
The explanation remains the same here ;-)
|
@karelhala can you merge this? |
karelhala
left a comment
karelhala
left a comment
There was a problem hiding this comment.
Overall looks good, just small change with logging click action to storybook action list.
| storiesOf('TextualSummary', module) | ||
| .add('GenericGroup', () => { | ||
| return ( | ||
| <GenericGroup items={genericGroupData.items} title={genericGroupData.title} onClick={e => null} /> |
There was a problem hiding this comment.
I don't like e => null as well, since it does not give any information to user of such storybook. Can you please use https://github.com/storybooks/storybook/tree/master/addons/actions so the action is logged at least into storybook action list?
|
Uh, sorry, timing :) so .. what Karel said :) |
|
Btw @martinpovolny if you want to show this storybook to outsiders, you can do it by using http://netlify.com from your repository and with build command as |
5 participants
The primary goal of this PR is to export more components that need to be used one-by-one in the ManageIQ.
Secondary, more tests and storybooks are provided.