This repository has been archived by the owner on Jan 16, 2022. It is now read-only.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
refactor: convert Engine component to hooks #233
refactor: convert Engine component to hooks #233
Changes from all commits
a42d019
c5cdc17
a0169f5
a870d1c
1559a9f
064a605
3f2349f
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's odd, I did not tried yet. I'll double check.
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 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really? 😞 I didn't test the new features yet, but I thought we could already use it ... so let's update prettier :D
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.
You can remove
button={true}
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.
Same here, You can remove button={true}
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 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think @ayusharma is referring to the fact that the
EngineListItem
styled component doesn't accept abutton
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://material-ui.com/api/list-item/
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.
regarding this
lint
rule, I believe we should discuss about it. I find this rule very annoying ... I also prefer to writebutton
only