Skip to content

Add new dom-elements-style-is-object rule (Fixes #715) #755

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

Merged

Conversation

petersendidit
Copy link
Contributor

@petersendidit petersendidit commented Aug 8, 2016

Fixes #715

@ljharb ljharb added the new rule label Aug 8, 2016
@@ -72,6 +73,7 @@ module.exports = {
}
},
rules: {
'react/dom-elements-style-is-object': 2,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we want to wait to enable this in the recommended configuration until the next major version bump, since doing so may end up breaking people's builds.

@yannickcr
Copy link
Member

Great! Can you fix the issues raised by @lencioni?

Also I'm not really a fan of the rule name: too long, should start by jsx- like the other rules.

jsx-style-object? Any other idea?

@ljharb
Copy link
Member

ljharb commented Aug 13, 2016

It shouldn't have jsx since it should apply to createElement DOM elements too.

@petersendidit
Copy link
Contributor Author

Yea been hoping to get back to fixing up this rule. Hoping I can do it sometime this weekend.

Yes the name is long but I think it accurately describes the rule.

@petersendidit petersendidit force-pushed the dom-elements-style-is-object branch from b0b6474 to f5ac0ec Compare August 13, 2016 17:52
@petersendidit
Copy link
Contributor Author

Checking that the style object values are stings has been pulled out and addressed the other comments.

@petersendidit petersendidit force-pushed the dom-elements-style-is-object branch 2 times, most recently from bb973bc to 460b887 Compare August 13, 2016 17:59
@yannickcr
Copy link
Member

@ljharb A lot of current JSX rules can also apply to createElement. So even if it also apply to createElement I still think it should be prefixed by jsx-, just by consistency with the other rules.

@ljharb
Copy link
Member

ljharb commented Aug 14, 2016

@yannickcr hm, I feel the opposite - that those rules should be renamed to not have jsx unless that's the only thing they apply to.

@petersendidit petersendidit force-pushed the dom-elements-style-is-object branch 2 times, most recently from 362248b to 8a9937c Compare August 21, 2016 19:36
@petersendidit
Copy link
Contributor Author

So what is the decision on the name? Are we going with jsx-style-object or are we sticking with the longer name?

@ljharb
Copy link
Member

ljharb commented Aug 21, 2016

I don't care about the length; style-prop-object would be fine with me too. I just think that jsx- should be reserved only for things that can only apply to jsx.

@yannickcr
Copy link
Member

Sorry for the delay.

those rules should be renamed to not have jsx unless that's the only thing they apply to

Yeah, in fact the jsx- rules should only be styling rules for JSX since the other ones can apply to creactElement too. A rename would be the solution but I'm not really a fan of renaming rules since it introduces a breaking change for almost no added value 😐. But that's another discussion 😉

I'm ok with style-prop-object 👍

@petersendidit petersendidit force-pushed the dom-elements-style-is-object branch from 8a9937c to ff6ab04 Compare August 22, 2016 01:21
@petersendidit
Copy link
Contributor Author

Renamed the rule. Let me know if there is anything else left to do.

@yannickcr yannickcr merged commit 7ed3b29 into jsx-eslint:master Aug 22, 2016
@yannickcr
Copy link
Member

Merged. Thank you!

@antoinerousseau
Copy link

does not work if i'm using componentWillReceiveProps's first argument (next props)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

5 participants