chore: expand coding standards #2581
Conversation
googlebot
added
the
cla: yes
| All public APIs must have user-facing comments. These are extracted and shown in the documation | ||
| on [material.angular.io](https://material.angular.io). | ||
|
|
||
| Private and internal APIs should have JsDoc when they are not obvious. Ultimately it is the purview |
There was a problem hiding this comment.
We should probably mention the @docs-private directive for excluding APIs that shouldn't be user-facing.
| Properties should have a concise description of what the property means: | ||
| ```ts | ||
| /** The label position relative to the checkbox. Defaults to 'after' */ | ||
| @Input() labelPosition: 'before' | 'after' = 'after'; |
There was a problem hiding this comment.
Perhaps, in general, we should be turning these into types? E.g. type MdLabelPosition = 'before' | 'after'; in order to allow users to annotate their code?
There was a problem hiding this comment.
Probably, but I don't think that is general enough for the coding guidelines.
| #### Provide function descriptions | ||
| For functions that are more complicated than a simple getter/setter, provide at least a brief | ||
| sentence explaining what the function does and/or _why_ it does something. | ||
| #### Typing |
There was a problem hiding this comment.
We should mention that, even though TypeScript infers it, we should include the method's return type for Dgeni's sake.
| ```ts | ||
| /** | ||
| * Opens a modal dialog containing the given component. | ||
| * @param component Type of the component to load into the dialog. |
There was a problem hiding this comment.
We don't want to mention the parameter type?
There was a problem hiding this comment.
The type is already in the TypeScript annotations. AFAIK Dgeni picks those up properly.
|
What about mentioning the |
| decorators, and when one of the arguments for the method is a native `Event` type, this preserved | ||
| type information can lead to runtime errors in non-browser environments (e.g., server-side | ||
| pre-rendering). | ||
|
|
||
|
|
||
| ### CSS | ||
|
|
There was a problem hiding this comment.
I agree with most of this, but the "don't use flex" part has always bothered me. Do we have an example of this weird baseline behavior that we're afraid of? It's hard to tell exactly what the issue is from reading the spec...
There was a problem hiding this comment.
TL;DR The text baseline display: flex and inline-flex is computed from the first child of the flex container, which is different from display: block and inline-block. If you put two of these next to each other, the browser will align the baselines and the elements will be misaligned.
Overall I think the messaging of "be cautious" with the two specific cases is good.
| ##### Methods | ||
| The name of a method should capture the action that is performed *by* that method. | ||
|
|
||
| ### Angular |
There was a problem hiding this comment.
Should we mention the forRoot replacement solution?
There was a problem hiding this comment.
I'm not sure that's general enough to belong in the coding guidelines.
jelbourn
force-pushed
the
coding-guidelines
branch
from
6b22f4e
to
f498258
Compare
|
Addressed comments |
| appropriate in your case. | ||
|
|
||
| For methods and properties that are part of a component's public API, all types must be explicitly | ||
| specified because our documentation tooling cannot current infer types in places where TypeScript |
There was a problem hiding this comment.
"cannot current infer" -> "cannot currently infer"
|
|
||
| Additionally, the `@docs-private` JsDoc annotation can be used to hide any symbol from the public | ||
| API docs. | ||
|
|
There was a problem hiding this comment.
What about the protected keyword? Do we prefer to make it public instead? A small note about best practices may be beneficial.
jelbourn
force-pushed
the
coding-guidelines
branch
from
f498258
to
e81d81a
Compare
|
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
All aboard!
@kara @mmalerba @andrewseguin @tinayuangao @crisbeto @EladBezalel @topherfangio @devversion @josephperrott
The coding standards were last updated just short of one year ago. Going forward, I want to update this doc much more regularly with insights that come out of code reviews.
Are there any conventions that we've been enforcing through code review that are not part of this doc?