Skip to content

Style Guide #1160

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

Merged
merged 33 commits into from
Sep 30, 2017
Merged

Style Guide #1160

merged 33 commits into from
Sep 30, 2017

Conversation

chrisvfritz
Copy link
Contributor

@chrisvfritz chrisvfritz commented Sep 30, 2017

It's done(ish)! 😄 🎉 Here's the hosted demo.

@chrisvfritz
Copy link
Contributor Author

Just doing a final code review, then I'll publish officially.


We might want to reuse this component, allowing users to maintain multiple lists (e.g. for shopping, wishlists, daily chores, etc). There's a problem though. Since every instance of the component references the same data object, changing the title of one list will also change the title of every other list. The same is true for adding/editing/deleting a todo.

Instead, we want each component instance to only manage its own data. For that to happen,each instance must generate a unique data object. In JavaScript, this can be accomplished by returning the object in a function:
Copy link
Member

Choose a reason for hiding this comment

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

Missing a space between happen, and each.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! Fixing.

```

``` js
// It's OK to use an object directly in a root
Copy link
Member

Choose a reason for hiding this comment

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

When it's "OK" to use an object in a root instance, I'd argue that it's still better to use a function for consistency and one less thing to remember. So maybe move this out of the "Good" section, for example as a note?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It'd be pretty controversial to discourage this, since we've demonstrated the pattern in half the pages of the docs for a long time. 😕

Copy link
Member

Choose a reason for hiding this comment

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

We don't discourage it, right? We can say that in the note, e.g.

"Since there's only one root Vue instance, it's OK to use a data object directly – in fact, that's what the demos on this site are doing. However, if you want consistency blahblah…"

Does it make sense?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think removing it from good would imply that it's not good, right? Why move it to a note, when this section is literally for demoing what's acceptable?

Detailed [prop definitions](https://vuejs.org/v2/guide/components.html#Prop-Validation) have two advantages:

- They document the API of the component, so that it's easy to see how the component is meant to be used.
- In development, Vue will warn you if a component is ever provided incorrectly formatted props, helping you catch potential sources of error.
Copy link
Member

Choose a reason for hiding this comment

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

provided with incorrectly formatted props?

Copy link
Contributor Author

@chrisvfritz chrisvfritz Sep 30, 2017

Choose a reason for hiding this comment

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

The "with" would only be appropriate if we also used an indirect object without "to" (source).


**Always use `key` with `v-for`.**

It's _always_ required on components, in order to maintain internal component state down the subtree. Even for elements though, it's a good practice to maintain predictable behavior, such as [object constancy](https://bost.ocks.org/mike/constancy/) in animations.
Copy link
Member

Choose a reason for hiding this comment

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

This sentence reads a bit weird to me. What's always required on components again?

Copy link
Contributor Author

@chrisvfritz chrisvfritz Sep 30, 2017

Choose a reason for hiding this comment

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

"It" refers to the previous sentence. Always using key with v-for is what's always required. Does that make sese?

Copy link
Member

Choose a reason for hiding this comment

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

I got it, but wouldn't it read more natural if we revise the section like this?

**Always use `key` with `v-for`.**
 
`key` with `v-for` is _always_ required on components, in order to maintain internal component state down the subtree. Even for elements though, it's a good practice to maintain predictable behavior, such as [object constancy](https://bost.ocks.org/mike/constancy/) in animations.

Since the previous sentence is actually a heading, I guess we can/should repeat the subject in the explaining paragraph.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I don't mind updating.

<ul>
<li v-for="todo in todos">
{{ todo.text }}
</td>
Copy link
Member

Choose a reason for hiding this comment

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

Why </td>?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because I forgot to change it when modifying the example. 😅 Good catch!

:key="todo.id"
>
{{ todo.text }}
</td>
Copy link
Member

Choose a reason for hiding this comment

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

Why </td>?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same deal here.

They'll never contain:

- global state (e.g. from a Vuex store)
-
Copy link
Member

Choose a reason for hiding this comment

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

What's this hyphen for?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For adding more items that I never needed to add. Removing this - thanks!


### Single-instance component names <sup data-p="b">strongly recommended</sup>

**Components that should only ever have a single active instance should begin with the `The` prefix, to denote that there can be only one.**
Copy link
Member

Choose a reason for hiding this comment

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

:/ I don't know about this. The seems weird to me, especially when used as a tag. I often go with something like App or Site instead, e.g. AppFooter or SiteHeader.

Copy link
Contributor Author

@chrisvfritz chrisvfritz Sep 30, 2017

Choose a reason for hiding this comment

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

The is the only one that unequivocally implies a single instance though. I've seen the App prefix commonly used for simple element wrappers, for example AppButton.

Copy link
Member

Choose a reason for hiding this comment

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

Hm… when I say "the avatar," I don't mean there's only one avatar in the whole app right? Maybe it's the avatar on the user profile? I don't know, this recommendation isn't very convincing to me TBH (but again maybe it's just me, as I'm not a native speaker).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That could be true, but we do have an international userbase, so that's not a reason to dismiss what you're saying. Is it the specific prefix or the idea of the convention in general that's not convincing you?


Since editors typically organize files alphabetically, all the important relationships between components are now evident at a glance.

You might be tempted to solve this problem differently, nesting all the search components under a "search" directory, then all the settings components under a "settings" directory. We only recommend this approach in very large apps of 100+ components, for these reasons:
Copy link
Member

Choose a reason for hiding this comment

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

IMO the number 100+ is pretty arbitrary. How about just mention "apps with a large number of components" instead?

Copy link
Contributor Author

@chrisvfritz chrisvfritz Sep 30, 2017

Choose a reason for hiding this comment

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

It is somewhat arbitrary, in the sense that we're using 100 rather than 99 or 101, but I worry that if we say "apps with a large number of components" people will just ask, "How many is a large number?" 😄

I've worked very comfortably with well over a 100 well-named components in a single directory, so I figured this was a relatively low, safe number for people to start considering breaking things out. What do you think?

Copy link
Member

@phanan phanan Sep 30, 2017

Choose a reason for hiding this comment

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

"How many is a large number?" 😄

"But that's the whole point – we've no idea! An app can be pretty complex with 50-ish components, when another with 150 can still be considered simple enough. It depends on what you're building, your skills, your team size etc. In other words, you know when to organize components into directories."

Copy link
Member

Choose a reason for hiding this comment

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

I guess we can rewrite this in a way to emphasize that this is subjective and user-dependent?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess part of my problem here is that I personally see nested component directories as an anti-pattern at virtually every size, even though many others would disagree. The problem is people reach for folders to help manage their complexity instead of giving their components better names, which has many more benefits. So I worry about giving apps that are too small an excuse to do this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, I just made a small change here from:

We only recommend this approach in very large apps of 100+ components [...]

to:

We only recommend considering this approach in very large apps (e.g. 100+ components) [...]

Gives people a little more leeway. 🙂

@@ -0,0 +1,82 @@
$style-guide-bad-bg = lighten(desaturate($red, 80%), 80%)
$style-guide-bad-text = darken(desaturate($red, 80%), 20%)
$style-guide-good-bg = lighten(desaturate($green, 80%), 85%)
Copy link
Member

Choose a reason for hiding this comment

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

It's kind of subjective, but personally I don't think the current background color is a bit too dark to indicate good. Maybe try something like #dbf1d9?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can tweak the colors after publishing, but I think this is good enough for now. The specific color you suggested looks very bright to me, like it's trying to highlight something rather than blending into the background. And since the rest of the page is using more subtle, desaturated colors, a more complete overhaul might be necessary to make something like that fit in.


**Component names should always be PascalCase in [single-file components](single-file-components.html) and string templates - but kebab-case in DOM templates.**

PascalCase in templates improves readability, because it's easier to tell at a glance what's an element and what's a component. Unfortunately, PascalCase is invalid for a custom element in HTML, so a DOM template must still use kebab-case.
Copy link
Member

@Justineo Justineo Sep 30, 2017

Choose a reason for hiding this comment

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

I think using kebab-case for component names is also quite clear to me (HTML tag names don't contain -) and we can always use kebab-case in template strings/SFCs/DOM templates, which might be even more consistent IMO. PascalCase component name smells a little bit like JSX...

<WelcomeMessage greeting-text="hi"/> in the examples looks really weird to me because it mixed two naming conventions in a very close position.

What about “kebab-case in templates, PascalCase in JSX”?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The PascalCase provides an extra layer of meaning in your app, by differentiating components from elements visually. Additionally, having the difference in convention between the two contexts (in-DOM components vs others) carries even more information, acting as a warning that you're in the DOM, so some limitations apply.

Using kekab-case everywhere doesn't have similar advantages, so "smells like JSX" isn't a good enough reason to reject a convention with clear benefits. 🙂


5. **Unique Attributes** (attributes that require unique values)
- `ref`
- `key`
Copy link
Member

Choose a reason for hiding this comment

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

Should key be together with v-for because of their correlation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, key isn't specific to v-for, but is used in a lot of contexts where uniquely identifying the element is useful.

Copy link
Member

Choose a reason for hiding this comment

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

OK that makes sense. I'm thinking of putting them together would make it easier to check for key after we created a v-for.

Copy link
Member

@Jinjiang Jinjiang left a comment

Choose a reason for hiding this comment

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

Totally reviewed. Love this style guide very much. And the "evil eyes" is funny 😄
Thanks.


**Component names should always be multi-word, except for root `App` components.**

This prevents conflicts with existing and future HTML elements, since all HTML elements are a single word.
Copy link
Member

Choose a reason for hiding this comment

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

Here is a ref may be useful: Valid custom element name in W3C Custom Elements Spec
The same concern for the name of W3C custom elements.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea!

```

``` html
<WelcomeMessage greeting-text="hi"/>
Copy link
Member

Choose a reason for hiding this comment

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

This example is a little strange for me because the tag name is PascalCase but attribute name is kebab-case. Maybe I prefer kebab-case tag name which is more natural for me. The second option is both PascalCases. The here is the 3rd. 😂

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, PascalCase is a convention used to identify instances in JavaScript. It applies to components, because they're really both JavaScript and instances. I think PascalCase would be strange for attributes, because they don't have instances. Does that make sense?

Copy link
Member

Choose a reason for hiding this comment

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

Ah, make sense. No problem. 👌

@chrisvfritz
Copy link
Contributor Author

chrisvfritz commented Sep 30, 2017

@phanan @Justineo @Jinjiang I'm merging now so that we can publish the beta on Vue 2's anniversary. 😄 That doesn't mean unresolved discussion has to end though. For anything you're still not satisfied with, please do open up an issue and we can discuss further. We won't be removing the beta tag until about a month from now.

@chrisvfritz chrisvfritz merged commit 88b4f25 into master Sep 30, 2017
kazupon added a commit to kazupon/vuejs.org that referenced this pull request Nov 24, 2017
* Style Guide (vuejs#1160)

* initial style guide prototype with examples

* clarify component style scoping in 3rd-party libs

* flesh out rule for complex computed properties

* clarify component style scoping

* flesh out self-closing components rule in style guide

* flesh out style guide entry for component name casing in JS/JSX

* complete draft for priority A rules

* complete draft for priority B rules

* style guide fixes after @KatieMFritz's review

* fix code block styles, especially on style guide

* fix wrong formatting for in-dom templates style rule

* simplify data function detailed explanation in style guide

* in style guide, add detailed explanation with example for keyed v-for

* in style guide, improve explanation of order of words in component names

* in style guide, link to SFC and JSX explanations

* WIP

* complete definitions for all style guide rules

* minor language tweaks to style guide

* update SG recommendation for nested component directories, courtesy of @Akryum

* fix SG typo in self-closing components example

* update SG component options order, tag @Akryum @posva

* add category tags to rule titles

* style guide round of edits and simplification

* move list rendering above conditionals in SG element attribute order

* add SG rule about empty lines in component/instance options

* fixed missing and added commas in SG example

* move style guide to its own page

* make exception to multi-word component names for App in SG

* add beta tag to style guide

* fix typos pointed out by @phanan

* clarify keyed v-for rule in SG

* update when component subdirectories are worth considering in SG

* add link to HTML spec in multi-word component names SG rule

* updates
@chrisvfritz chrisvfritz deleted the style-guide branch May 23, 2018 17:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants