-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
[New] jsx-sort-props
: support multiline prop groups
#3198
[New] jsx-sort-props
: support multiline prop groups
#3198
Conversation
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.
Rather than two mutually exclusive boolean props, this would be better as an enum value for "multiline" - ignore (default), first, or last, perhaps?
a={{ | ||
aA: 1, | ||
}} | ||
b | ||
inline={1} |
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.
We have a limitation with the current implementation here, whenever conflicting groups are in order between themselves, but in disorder with other props, only the group with higher hierarchy will be reported.
For example here we enabled { multiline: 'last', shorthandLast: true }
, but only the shorthand prop is being reported as an error, since multiline thinks that it's in order.
I feel like there's no easy way to fix this unless we change the way in which jsx-sort-props
checks and order the props, so that it check props by groups instead of the direct adjacent neighbors of each prop. I wouldn't mind working on that, but I feel like that's out of the scope of this PR...
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 agree; that sounds like a great improvement, but belongs in another PR.
39b452b
to
c46efb8
Compare
@ljharb I was able to work on the given feedback, now the property is an enum rather than two mutually exclusive props, and the tests are now expecting line numbers for each specific error. There was a little issue regarding how the |
a={{ | ||
aA: 1, | ||
}} | ||
b | ||
inline={1} |
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 agree; that sounds like a great improvement, but belongs in another PR.
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.
Nicely done, and very thorough.
85a55f0
to
446c5a3
Compare
Codecov Report
@@ Coverage Diff @@
## master #3198 +/- ##
=======================================
Coverage 97.62% 97.63%
=======================================
Files 121 121
Lines 8381 8416 +35
Branches 3011 3025 +14
=======================================
+ Hits 8182 8217 +35
Misses 199 199
Continue to review full report at Codecov.
|
446c5a3
to
f47deef
Compare
One more change, because node < 6 doesn't have array |
Hello 👋
This PR Adds a new multiline group for the
jsx-sort-props
rule. Related to the issue: #3170The shorthand and callback rules take precedence over the multiline rule set