refactor: convert Engine component to hooks #233
Conversation
Codecov Report
@@ Coverage Diff @@
## master #233 +/- ##
==========================================
- Coverage 88.98% 88.85% -0.14%
==========================================
Files 139 139
Lines 926 915 -11
Branches 159 160 +1
==========================================
- Hits 824 813 -11
Misses 88 88
Partials 14 14
|
| 'node-JS': engines.node, | ||
| 'NPM-version': engines.npm, | ||
| }; | ||
| const engines = packageMeta && packageMeta.latest && packageMeta.latest.engines; |
There was a problem hiding this comment.
I saw that TypeScript 3.7 is specified in package.json, but ?. doesn't seem to be supported by the tooling yet :(
There was a problem hiding this comment.
That's odd, I did not tried yet. I'll double check.
There was a problem hiding this comment.
I think it was prettier that was giving me issues, 1.19 with support for 3.7 isn't released yet: https://github.com/prettier/prettier/blob/master/CHANGELOG.unreleased.md
There was a problem hiding this comment.
Really? 😞 I didn't test the new features yet, but I thought we could already use it ... so let's update prettier :D
| {engines.node && ( | ||
| <Grid item={true} xs={6}> | ||
| <List subheader={<StyledText variant={'subtitle1'}>{'node JS'}</StyledText>}> | ||
| <EngineListItem button={true}> |
| {engines.npm && ( | ||
| <Grid item={true} xs={6}> | ||
| <List subheader={<StyledText variant={'subtitle1'}>{'NPM version'}</StyledText>}> | ||
| <EngineListItem button={true}> |
There was a problem hiding this comment.
Same here, You can remove button={true}
There was a problem hiding this comment.
This should be enforced by eslint, I'd suggest let it go and add the plugin or rule and change all places at a time.
There was a problem hiding this comment.
I think @ayusharma is referring to the fact that the EngineListItem styled component doesn't accept a button prop.
I left it because removing it breaks the snapshot tests, which I consider bad practice when doing a refactor like this.
There was a problem hiding this comment.
Hi all! I just checked and the ListItem component from Material UI does accepted the prop button..we should change it if we don't want to use the ButtonBase only :
So IMO it's fine if you leave like it is now...we will check all cases in our design system
There was a problem hiding this comment.
There was a problem hiding this comment.
regarding this lint rule, I believe we should discuss about it. I find this rule very annoying ... I also prefer to write button only
There was a problem hiding this comment.
@bighuggies thanks for contributing :-) I really like that you added a print of your changes.. I will do the same next time. Having something visual helps during review
Type: refactor
Description:
Refactored
<Engine />to a functional component using hooks, similar to #150 and #156Also unrolled the
reducefunction, improving readability.Snapshots are the same and also tested locally by updating the hardcoded engine data:
Resolves #116