-
-
Notifications
You must be signed in to change notification settings - Fork 680
Style Guide Collaboration #77
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
Comments
👍 I like it |
Good idea ❤️ |
@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. |
@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. 🙂 |
This is amazing! I love it. A couple of notes: (these are so minor)
I think this one could do with an example.
Clarification needed: I know a good number of people who won't scope the topmost
Can you please explain the inconsistency here? This is fantastic, I'm sure it's going to be a great resource for people. |
Perfect! Thank you. |
💪 Love this! Can't wait to have a linter to check them instead of doing manually in every merge request! Some suggestions: Props CasingA 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:
and
We created an entry to use the latter: https://docs.gitlab.com/ce/development/fe_guide/style_guide_js.html#naming
|
@filipalacerda can you make tickets with new rule proposition and i will work on this tonight (i have no idea how to name them). |
When it comes to templates I prefer having the attributes in such order: Example:
Could also order based on the native attributes first and directives/props/events second. |
@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. |
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:
Thoughts?
|
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. |
Will do. 🙂 |
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.
|
@fatihacet that's what I've been doing too. Enforcing this would be nice. |
@fatihacet @piehei We actually have a rule addressing attribute order in the style guide. I'd love your feedback. 🙂 |
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 |
@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:
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? |
@kaicataldo #205 creates these rule sets.
@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, 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. 🙂 |
@chrisvfritz I've been looking at
I would expect this example to not throw a warning by default. What do you think? |
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 We should also reprioritise other propositions in favor of the official style guide now :) 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 |
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.
I agree that enforcing rules in the style guide should take priority, as that will probably be what users expect.
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
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.
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. |
I'm glad we're on the same page here ✋ |
@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). |
@michalsnik Looks fantastic! I just went through and reviewed them all. Thank you for organizing them to make it so easy. 😄 |
I get page not found. is this dead? |
also the branch cited is not found |
@Maxhodges That was never a live link (notice the |
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 |
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.
The text was updated successfully, but these errors were encountered: