Skip to content
This repository has been archived by the owner on Dec 15, 2020. It is now read-only.

Update eslint #337

Merged
merged 9 commits into from
Oct 21, 2016
Merged

Update eslint #337

merged 9 commits into from
Oct 21, 2016

Conversation

mikestone14
Copy link
Contributor

No description provided.

@marpaia
Copy link
Contributor

marpaia commented Oct 21, 2016

This looks really great, thanks for tackling this. I'm not super familiar, so can you help me understand why PropTypes.object is a bad thing that eslint doesn't like and why some instances are moved into the userInterface object and some have an // eslint-disable-line comment?

@mikestone14
Copy link
Contributor Author

@marpaia my guess is that they want you to be more specific to ensure the object props are what the app is expecting. As a guy who loves types I figure you're into that 😄 .

I think this is also one step closer to using TypeScript as we're defining interfaces for many props. We have to use // eslint-disable-line on props related to styles. Technically we don't have to but otherwise we'd have to shape the style props to define all styles included which is going to add a ton of code and is not super important.

@marpaia
Copy link
Contributor

marpaia commented Oct 21, 2016

OH, I get it, this is really neat and way more typesafe! That's so cool :)

@marpaia
Copy link
Contributor

marpaia commented Oct 21, 2016

I see your logic with not setting interfaces for the style prop as well, given that they can be so different and change so often. This is especially true given that @kyleknighted is probably going to be refactoring styles to use SCSS anyway. I'm good with merging this, thanks for doing this!

@mikestone14
Copy link
Contributor Author

Sounds good thanks for the review @marpaia. I haven't seen people use PropType interfaces this way before but I think it could be a good convention to use internally. Maybe one day we'll even build a component using Typescript. And you're right, hopefully all those style props go away soon in which case we can define interfaces for all object props.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants