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

Style Guide Collaboration #77

Closed
chrisvfritz opened this issue Jul 14, 2017 · 32 comments
Closed

Style Guide Collaboration #77

chrisvfritz opened this issue Jul 14, 2017 · 32 comments

Comments

@chrisvfritz
Copy link
Contributor

chrisvfritz commented Jul 14, 2017

I'm developing an official Vue style guide and although not all rules there will be enforceable through this plugin, I think it makes sense to collaborate here, especially for those rules that are enforceable. Right now, I'm thinking the style guide could serve as the official documentation for any recommended rules in this plugin and these two resources could be published together.

You can see the current progress in this branch, with the live result here. Any feedback/contributions are welcome and rules are not set in stone, so don't panic if you see something you disagree with. 😄

I've also not kept up-to-date with the conversation in various issues here, so I apologize if some of it is out of sync with plans you've been making. Please ping me on discussions you think I should know about, or if you'd like my input.

@armano2
Copy link
Collaborator

armano2 commented Jul 14, 2017

👍 I like it

@mysticatea
Copy link
Member

Good idea ❤️
I think good if we have predefined configs about the priority A to D.
Also, the style guide is the best source of rule ideas.

@chrisvfritz
Copy link
Contributor Author

chrisvfritz commented Jul 17, 2017

@mysticatea Agreed, though inspiration can also go both ways. If you have any rules from the plugin that should be added, or if a recommended rule in the plugin conflicts, please discuss it here.

Feedback on the categories I've created are also welcome - as well as feedback on which category each rule belongs in. I tried to put a lot of research and thought into them, but this is still the first community style guide I'm creating and I'm sure there are considerations I'm not experienced enough to foresee.

@chrisvfritz
Copy link
Contributor Author

chrisvfritz commented Jul 17, 2017

@filipalacerda Also tagging you here, as I know you've thought about this quite a lot at Gitlab and your internal style guide was actually a source of inspiration for the official community one. 🙂

@sdras
Copy link
Member

sdras commented Jul 17, 2017

This is amazing! I love it.

A couple of notes: (these are so minor)

Complex computed properties
Whenever possible, computed properties should be split into as many named values as possible.

I think this one could do with an example.

Component style scoping
Component styles should be scoped.

Clarification needed: I know a good number of people who won't scope the topmost App.vue component tag, but every subsequent component thereafter, are we deciding that this is an antipattern too? If not, it might be worth detailing this a bit more.

Self-closing components
Components with no content should be self-closing in single-file component or string templates, but never in DOM templates.

Can you please explain the inconsistency here?

This is fantastic, I'm sure it's going to be a great resource for people.

@chrisvfritz
Copy link
Contributor Author

chrisvfritz commented Jul 17, 2017

@sdras Great comments! I just updated those three entries to (hopefully) resolve your concerns. Let me know how I did. 😄

@sdras
Copy link
Member

sdras commented Jul 17, 2017

Perfect! Thank you.

@filipalacerda
Copy link
Contributor

💪 Love this!
GitLab style guide goes mainly in the same direction - https://docs.gitlab.com/ce/development/fe_guide/style_guide_js.html.

Can't wait to have a linter to check them instead of doing manually in every merge request!

Some suggestions:

Props Casing

A rule for pros name casing in templates, similar to "Component name casing in templates", it is something we struggle with in our codebase, we often see:

<component myProp="prop"></component>

and

<component my-prop="prop"></component>

We created an entry to use the latter: https://docs.gitlab.com/ce/development/fe_guide/style_guide_js.html#naming

name property casing

We haven't decide on a rule to this one which makes us have components without it, other with it sometimes in camelCase others in PascalCase.
Might be worth adding a rule in priority B or C to aim for consistency

{
  name: 'MyComponent'
}
{
  name: 'myComponent'
}

Let's get this done 💪 I would love to help!

@armano2
Copy link
Collaborator

armano2 commented Jul 18, 2017

@filipalacerda can you make tickets with new rule proposition and i will work on this tonight (i have no idea how to name them).

@shentao
Copy link
Member

shentao commented Jul 18, 2017

When it comes to templates I prefer having the attributes in such order: directives -> attrs -> :attrs -> @events.

Example:

<input 
  v-for="name of names" 
  v-model="name" 
  type="text" 
  :disabled="isDisabled(name)" 
  @change="validate(name)"
/>

Could also order based on the native attributes first and directives/props/events second.

@filipalacerda
Copy link
Contributor

@armano2 created #92 and #93 :)

@chrisvfritz
Copy link
Contributor Author

@filipalacerda I like both of those ideas. 🙂 I'll also add a rule about max attributes per line and take another look at Gitlab's style guide to see what else you added since I last looked at it.

@chrisvfritz
Copy link
Contributor Author

chrisvfritz commented Oct 3, 2017

I just released a beta of the Vue style guide, with plans to remove the beta tag in mid to late October.

@mysticatea @armano2 @michalsnik Do you think that could be a good time for an official v3 release of this plugin? By that time, I'm thinking we could ensure that:

  • vue/recommended is consistent with rules in the style guide
  • when an ESLint rule has an equivalent in the style guide, we link to the style guide rule - and also link to the ESLint rule from the style guide

Thoughts?

EDIT: Note that I do not expect the ESLint plugin to be able to enforce all style guide rules. I'd just like to be able to communicate to users what they can expect to be enforced here and what they will have to enforce manually. For rules that are technically enforceable by ESLint, but would take a while to develop, it might be best to wait on those to ensure we can publish what we have.

@michalsnik michalsnik mentioned this issue Oct 3, 2017
30 tasks
@michalsnik
Copy link
Member

Hey @chrisvfritz , I saw a tweet about it already. Very good work! 💪 🚀

I think we can do that. I was quite busy recently thus didn't have enough time to push things here but it looks like we have only 3 things left to do that block us before the official release, and some of them or all are already addressed and wait for review which I'm going to do very soon. In the meanwhile we can discuss the potential config, so it's in line with the styleguide and also update documentations.

I'll better focus on pushing forward everything that waits in queue in official release milestone first. And if you're able to prepare a basic PR with proposed configuration on which we can further iterate that'd be really great :) If not, I'll probably finally have some time during upcoming weekend, so I can do that too.

@chrisvfritz
Copy link
Contributor Author

And if you're able to prepare a basic PR with proposed configuration on which we can further iterate that'd be really great

Will do. 🙂

@fatihacet
Copy link

@shentao Re: #77 (comment)

Another GitLab dev here and at GitLab we were discussing to have Vue related attributes before the native attributes. It makes it easier to see the Vue attributes at a glance, so you don't need to skim through all attributes to find out whether that element already has the Vue attribute you are looking for.

<input 
  v-for="name of names" 
  v-model="name"
  :disabled="isDisabled(name)" 
  @change="validate(name)"
  type="text" 
  placeholder="Name"
/>

@piehei
Copy link

piehei commented Oct 5, 2017

@fatihacet that's what I've been doing too. Enforcing this would be nice.

@chrisvfritz
Copy link
Contributor Author

@fatihacet @piehei We actually have a rule addressing attribute order in the style guide. I'd love your feedback. 🙂

@kaicataldo
Copy link

Hi! Where do things stand with this? Is there a checklist or something to look at to see what is done and work is still outstanding? Looking forward to this!

Also, apologies if this has been discussed somewhere else, but are there plans to create different sets of rules for the different tiers of importance (Essential, Strongly Recommended, etc.) or is the plan to just add it to the recommended set right now?

@matt-oconnell
Copy link

@chrisvfritz I started work with @depew on an eslint rule for attribute order. I wonder if the attribute order is too fine-grained. In working with Vue, we've often enforced an order more like:

1) refs
2) directives (anything with a v- prefix excluding bind)
3) event listeners (@)
4) bound props (:)
5) generic attributes (href, src, class...)

10 grouping types seems a little much to remember but that's just my own opinion. With the above ordering, we don't need to remember the subsets but rather we can simply group attributes with matching prefixes together. Maybe @fatihacet @piehei @kaicataldo have an opinion on this?

@chrisvfritz
Copy link
Contributor Author

are there plans to create different sets of rules for the different tiers of importance (Essential, Strongly Recommended, etc.) or is the plan to just add it to the recommended set right now?

@kaicataldo #205 creates these rule sets.

10 grouping types seems a little much to remember

@matt-oconnell While that grouping is easier to remember, I don't think it's very useful, as so many of the most important distinctions are lost. For example, :is would appear near the bottom in group 4, making it easy to miss, but it completely changes the context for every other attribute. A v-if would also be allowed to appear before a v-for, which could imply that they're interpreted in that order.

Either way, I think this is something that should be fixable, which means no one will even have to remember it if they use this plugin. 🙂

@michalsnik
Copy link
Member

@chrisvfritz I've been looking at max-attributes-per-line rule today and I wonder whether the default value for singleline option shouldn't be 2 or 3, not 1. Consider the following common case:

<div v-for="n in 10" :key="n">
</div>

I would expect this example to not throw a warning by default. What do you think?

@michalsnik
Copy link
Member

I'm going through the styleguide and will add more rules' propositions that we could implement in next releases. I'll mark them with label Present in Style Guide and it would be our next priority after releasing stable 4.0.

We should also reprioritise other propositions in favor of the official style guide now :)
Please take a look guys at all propositions and comment, so we can decide together what do we want to add and what not. Maybe there is something even worth considering adding to official style guide @chrisvfritz :)

And actually I think that the flow for adding new rules in this repository should include decision process on the style guide level. If something is worth adding to official style guide - that means it's definitely worth the hassle to make a rule for it too. Do you agree?

I'll create another label Style Guide candidate for marking propositions that we should consider adding to the style guide first.

@chrisvfritz
Copy link
Contributor Author

I'm going through the styleguide and will add more rules' propositions that we could implement in next releases. I'll mark them with label Present in Style Guide and it would be our next priority after releasing stable 4.0.

That sounds like a good plan to me. 🙂 We also still have to add links back and forth between the style guide and rules in this repo.

We should also reprioritise other propositions in favor of the official style guide now :)
Please take a look guys at all propositions and comment, so we can decide together what do we want to add and what not.

I agree that enforcing rules in the style guide should take priority, as that will probably be what users expect.

Maybe there is something even worth considering adding to official style guide

New additions to the style guide page are definitely welcome!

There will be some rules we can't/shouldn't enforce here, such as those typically in Priority D: Use with Caution - how do we detect if a feature was not used with caution? 😄

And similarly, some rules here will be implied rules in the style guide, but not explicitly mentioned, such as no-dupe-keys. That one's probably obvious and straight-forward enough so that no justification/exploration is needed, but it's a great ESLint rule because it could be easy to make the mistake.

And actually I think that the flow for adding new rules in this repository should include decision process on the style guide level. If something is worth adding to official style guide - that means it's definitely worth the hassle to make a rule for it too. Do you agree?

I 90% agree. 🙂 In cases where implementing an ESLint rule would be especially labor-intensive, I think it also makes sense to take into account how common violations are (how much time are we actually saving users?). If the value for the effort is relatively low, we might want to make it a lower priority.

I'll create another label Style Guide candidate for marking propositions that we should consider adding to the style guide first.

Good idea. And I like that flow: add to the style guide first, when appropriate, then create an ESLint rule. Creating the more detailed explanation in the style guide will give us a chance to fully think through the problem and all possible acceptable styles, so that creating the ESLint rule will be easier.

@michalsnik
Copy link
Member

I'm glad we're on the same page here ✋

@michalsnik
Copy link
Member

@chrisvfritz I finished adding rules' propositions regarding current state of the Style Guide:

Take a look and give a thumbs up if you also agree on them :)

I also cleared out all issues, and it looks like we have only 5 rules propositions that are not present in style guide (mostly because they're too obvious).

@chrisvfritz
Copy link
Contributor Author

chrisvfritz commented Nov 27, 2017

@michalsnik Looks fantastic! I just went through and reviewed them all. Thank you for organizing them to make it so easy. 😄

@Maxhodges
Copy link

I get page not found. is this dead?
https://vue-style-guide-dev.surge.sh/v2/guide/style-guide.html

@Maxhodges
Copy link

@chrisvfritz
Copy link
Contributor Author

@Maxhodges That was never a live link (notice the dev in the subdomain). The published style guide is at vuejs.org/v2/style-guide.

@michalsnik
Copy link
Member

Now that we have separate issues for different recommendation from style guide I think we can close this and continue discussions there. If something is worth to be added to Style Guide we're going to add badge Styleguide Candidate on the given proposition. Thank you all for your contribution :)

@guohuihot
Copy link

image
How can we do this?

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

No branches or pull requests