Skip to content
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

Rewrite code to make xo happy without any rules #730

Closed
8 of 12 tasks
leo opened this issue Sep 21, 2016 · 14 comments
Closed
8 of 12 tasks

Rewrite code to make xo happy without any rules #730

leo opened this issue Sep 21, 2016 · 14 comments
Labels
help wanted Contributions wanted towards the issue ❣️ Priority: High Issue is of High priority 🎨 Type: Enhancement Issue or PR is an enhancement request/proposal for Hyper

Comments

@leo
Copy link
Contributor

leo commented Sep 21, 2016

The long-term goal is to comply with all of xo's standards, so that we can remove these rules. I already made the code comply to many rules, but these are left.

@leo leo added 🎨 Type: Enhancement Issue or PR is an enhancement request/proposal for Hyper help wanted Contributions wanted towards the issue 💚 Priority: Low Issue is "nice-to-have" or not important to the core app labels Sep 21, 2016
@davegomez
Copy link
Contributor

I can work on this!

@davegomez
Copy link
Contributor

davegomez commented Sep 23, 2016

The changes for this issue are massive in some cases. I'm planning to send one PR for each rule fulfilled.

@leo
Copy link
Contributor Author

leo commented Sep 23, 2016

@davegomez Cool, thanks! 😘

@ekmartin
Copy link
Contributor

react/prop-types: Since we don't have any prop types from before, wouldn't it be better to add flow? It can be added incrementally as well, and we don't have to use it for anything else than props.

react/no-danger: #723 (comment) (the only place we use dangerouslySetInnerHTML it's the only way we can do it, and as the link mentions I think the name covers it more than enough by itself)

@sindresorhus
Copy link
Contributor

Context of why those rules should be enabled: #723 (comment)

@sindresorhus
Copy link
Contributor

react/no-danger: #723 (comment) (the only place we use dangerouslySetInnerHTML it's the only way we can do it, and as the link mentions I think the name covers it more than enough by itself)

Then I would recommend just disabling it inline where it's used with a ESLint directive:

// eslint-disable-line react/no-danger

@davegomez
Copy link
Contributor

So are we gonna add the Proptypes or don't?

And I agree with @sindresorhus that we should use the inline comment to disable the react/no-danger rule.

This was referenced Oct 2, 2016
@maxdeviant
Copy link
Contributor

maxdeviant commented Oct 2, 2016

@leo @sindresorhus How do we want to handle quote-props?

It seems like the only place we are using them is for CSS pseudo-classes (e.g., :hover), and it seems rather silly to have to quote all the properties off of that 😅

Should we disable with inline comments? I don't suppose we could switch the xo rule to as-needed?

@sindresorhus
Copy link
Contributor

I don't suppose we could switch the xo rule to as-needed?

@maxdeviant Done: xojs/eslint-config-xo@abe5ed6 I've been meaning to change it. It sounded good initially, but became annoying after awhile. Will be part of the next XO release being released soon (in a few days to a week).

@maxdeviant
Copy link
Contributor

@sindresorhus Okay, sounds good :)

I'll double check, but once we update XO then we should able able to re-enable the rule and cross quote-props off the list 🎉

@maxdeviant
Copy link
Contributor

@leo As per @sindresorhus' comment, no-nested-ternary no longer applies, so we can remove from this list once we update our version of XO.

@leo
Copy link
Contributor Author

leo commented Oct 6, 2016

@maxdeviant Got it! But it looks like he hasn't released it yet...

@maxdeviant
Copy link
Contributor

@leo My bad, forgot to mention that part 😅

@leo leo added ❣️ Priority: High Issue is of High priority and removed 💚 Priority: Low Issue is "nice-to-have" or not important to the core app labels Oct 13, 2016
@leo
Copy link
Contributor Author

leo commented Feb 7, 2018

We don't use xo in Hyper anymore... 😊

@leo leo closed this as completed Feb 7, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Contributions wanted towards the issue ❣️ Priority: High Issue is of High priority 🎨 Type: Enhancement Issue or PR is an enhancement request/proposal for Hyper
Projects
None yet
Development

No branches or pull requests

5 participants