8000 Add same order support on multiple types of attributes. by tinymins · Pull Request #471 · vuejs/eslint-plugin-vue · GitHub
[go: up one dir, main page]
Skip to content

Add same order support on multiple types of attributes. #471

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&rd 8000 quo;, 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
Merged
merged 1 commit into from
Jul 30, 2018
Merged

Add same order support on multiple types of attributes. #471

merged 1 commit into from
Jul 30, 2018

Conversation

tinymins
Copy link
Contributor
@tinymins tinymins commented May 5, 2018

No description provided.

@michalsnik
Copy link
Member

Hey @tinymins Thanks for PR!

Can you provide more informations about this change?

  • Is it a bug fix or improvement?
  • What was the motivation?
  • What problem does it solve?
  • Is it related to any posted issue?
  • Can you provide test cases that would fail before this change?

8000
@tinymins
Copy link
Contributor Author

Thanks,

  1. Is it a bug fix or improvement?
    Improvement.

  2. What was the motivation?
    In my project we want to sort attributes but we want some different kinds of attributes can have same priority. For example:

    <component id="a1" class="c1" :class="c2" style="color: black"></component>

    In the code above, we want to force id before class and style, but we don't care if :class is before or after class.

  3. What problem does it solve?
    When lint sort attributes, some kinds of attributes can have same priority.

  4. Is it related to any posted issue?
    Maybe not? I don't know if anyone else has the same problem.

  5. Can you provide test cases that would fail before this change?
    Just like the example above, that case cannot be solved before this change, thus, we have to turn off this rule, because we do not want to force let :class(computed prop) before or after class(static prop).

@tinymins
Copy link
Contributor Author

What's more, I have an other improvement idea for this rule. If an attribute's type is not listed in attribute-order, it will be reported as wrong order if this attribute exist, no matter what position it is at. I think this is not right, attribute not be listed in order should be free at any position.

@michalsnik
Copy link
Member

Alright @tinymins ! Thanks for clearing it out.

I think that your case was covered in #421, as we got rid of BINDING group there and treat bindings and other attrs equally. (It just waits for the major release).

If you however still want to ship support for grouping properties, I have to ask you to add tests covering the proposed enhancement - right here: https://github.com/vuejs/eslint-plugin-vue/blob/master/tests/lib/rules/attributes-order.js
Although given the change in #421 I'm not sure it's still that relevant. What do you think?

@tinymins
Copy link
Contributor Author

Thanks, @michalsnik
I've read the file changes in #421, and I think my PR will get conflicted with #421. When will #421 got merged into master branch?

@michalsnik
Copy link
Member

Hard to tell, maybe in week or two. Once we're done fixing current bugs and start preparing 5.0-beta

michalsnik
michalsnik approved these changes Jul 30, 2018
View reviewed changes
Copy link
Member
@michalsnik michalsnik left a comment

Choose a reason for hiding this comment

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

LGTM 👍 GJ @tinymins

@michalsnik michalsnik merged commit 341bccf into vuejs:master Jul 30, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@michalsnik michalsnik michalsnik approved these changes

Assignees
No one assigned
Labels
accepted proposition enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@tinymins @michalsnik
0