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.

Sign up for GitHub

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

Jump to bottom

[New] style-prop-object: Add allow option #1819

Merged
merged 1 commit into from
Feb 15, 2020
Merged

[New] style-prop-object: Add allow option #1819

merged 1 commit into from
Feb 15, 2020

Conversation

hornta
Copy link

@hornta hornta commented Jun 9, 2018

Fixes #1813

@ljharb
[New] style-prop-object: Add allow option
edc5fe2 
Fixes #1813

Co-Authored-By: Erwin <evosch@users.noreply.github.com>
Co-Authored-By: hornta <4553604+hornta@users.noreply.github.com>
Co-Authored-By: Anthon Fredriksson <anthonnorrby@gmail.com>

fix trailing comma causing eslint error
@jseminck
Copy link
Contributor

jseminck commented Jun 9, 2018 •
edited
Loading

I think that we should also make sure that React.createElement(MyCustomElem, { style: "myStyle" }) works if "MyCustomElem" is an exception?

@hornta
Copy link
Author

hornta commented Jun 9, 2018

@jseminck Oh yes of course. Thank you. I'll try to cover that aswell.

@hornta
Copy link
Author

hornta commented Jun 9, 2018

@jseminck I've updated this PR with React.createElement.
I've also managed to get 100% coverage in this rule 👍

@jseminck
Copy link
Contributor

jseminck commented Jun 9, 2018 •
edited
Loading

Cool. LGTM from my side, as I attempted to add this improvement myself, and my solution was very similar. 😄

I haven't been involved in this project for a while though so it will need a review from someone else as well.

@ljharb ljharb mentioned this pull request Jun 12, 2018
Closed
ljharb
ljharb reviewed Jun 12, 2018
View reviewed changes
Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

Overall this seems good, thanks

.gitignore Outdated Show resolved Hide resolved
lib/rules/style-prop-object.js Outdated
const componentName = node.arguments[0].name;

// allowed list contains the name
if (allowed.indexOf(componentName) > -1) {
Copy link
Member

Choose a reason for hiding this comment

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

we should be able to make a Set on line 38, so this doesn't need to be O(n)

lib/rules/style-prop-object.js Outdated
// parent element is a JSXOpeningElement
if (node.parent && node.parent.type === 'JSXOpeningElement') {
// store parent element
const jsxOpeningElement = node.parent;
Copy link
Member

Choose a reason for hiding this comment

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

this can be stored prior to the "if", so we don't have to extract it multiple times.

@evosch evosch mentioned this pull request Dec 6, 2018
Closed
@ljharb
Copy link
Member

ljharb commented Dec 7, 2018

@hornta would you mind checking the "allow edits" checkbox on the right hand side of the PR?

@hornta
Copy link
Author

hornta commented Dec 7, 2018

@hornta would you mind checking the "allow edits" checkbox on the right hand side of the PR?

Done. I totally forgot this one! 😨

@evosch
Copy link

evosch commented Dec 7, 2018

Sorry for the duplicate pull request, I'm a bit new to this. How should I get the changes in? Do a PR on hornta/eslint-plugin-react?

@hornta hornta closed this Dec 7, 2018
@ljharb ljharb reopened this Dec 7, 2018
@ljharb
Copy link
Member

ljharb commented Dec 7, 2018

@evosch yep! Then you can post the link here, and i can merge it in to the PR if needed.

@ljharb
Copy link
Member

ljharb commented Jan 4, 2019

Just noticed https://github.com/hornta/eslint-plugin-react/pull/1 was filed. I'll pull that in soon.

ljharb
ljharb reviewed Jan 4, 2019
View reviewed changes
Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

Seems reasonable overall

lib/rules/style-prop-object.js Outdated Show resolved Hide resolved
@ljharb ljharb requested review from yannickcr and EvHaus January 13, 2019 07:55
EvHaus
EvHaus approved these changes Jan 13, 2019
View reviewed changes
Copy link
Collaborator

@EvHaus EvHaus left a comment

Choose a reason for hiding this comment

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

LGTM

@koba04
Copy link

koba04 commented Mar 26, 2019

Are there any blockers to be merged this?

EvHaus
EvHaus approved these changes Mar 26, 2019
View reviewed changes
Copy link
Collaborator

@EvHaus EvHaus left a comment

Choose a reason for hiding this comment

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

LGTM. Seems pretty weird for components to override default React behavior for the style prop, but I guess if there's a good use case for it, we should support it.

@ron23
Copy link

ron23 commented Feb 4, 2020

Hey, @hornta I too would love this to be merged. Any plans moving forward with this?

@ljharb ljharb changed the title Add allow option in style-prop-object [New] style-prop-object: Add allow option Feb 15, 2020
@ljharb ljharb merged commit edc5fe2 into jsx-eslint:master Feb 15, 2020
@ljharb ljharb mentioned this pull request Feb 25, 2020
Closed
@gone-skiing gone-skiing mentioned this pull request Mar 6, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ljharb ljharb ljharb approved these changes

@EvHaus EvHaus EvHaus approved these changes

@yannickcr yannickcr Awaiting requested review from yannickcr

Assignees
No one assigned
Labels
enhancement rule
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Allow exceptions for style-prop-object
7 participants
@hornta @jseminck @ljharb @evosch @koba04 @ron23 @EvHaus