Skip to content

chore: expand and polish cdk docs #7675

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 1 commit into from
Oct 27, 2017
Merged

Conversation

jelbourn
Copy link
Member

  • Add JsDocs where missing
  • Adds and polishes overviews for main cdk subpackages
  • Tweaks dgeni to disambiguate between material and cdk output (e.g. with "table").
  • Tweaks markdown process to disambiguate between material and cdk output
  • Fix an issue where method return types were missing from all api docs
  • Correct some any property types in cdk/stepper
  • Correct table trackBy JsDoc being omitted because it was on the setter

@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Oct 10, 2017
@jelbourn jelbourn added docs This issue is related to documentation pr: needs review labels Oct 10, 2017
});

/** Generates html files from the markdown overviews and guides. */
task('markdown-docs-cdk', () => {
Copy link
Member Author

Choose a reason for hiding this comment

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

@crisbeto or @devversion I know this is wrong but I don't know the right way to fix it

(disambiguating material and cdk output)

Copy link
Member

Choose a reason for hiding this comment

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

Yeah I see. Would have been nice to name the other task markdown-docs-material or something, but this is not really a priority right now.

Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure a better way to resolve this right now, left a TODO

@@ -27,7 +28,7 @@ import {Direction, Directionality} from './directionality';
exportAs: 'dir',
})
export class Dir implements Directionality {
/** Layout direction of the element. */
/** @docs-private */
_dir: Direction = 'ltr';
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't be needed since its prefixed with an underscore.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done


#### React to changes to the viewport
The `observe` method is used to get an observable stream that will emit whenever one of the given
media queries would have a different results.
Copy link
Member

Choose a reason for hiding this comment

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

Not too sure, would have different results (dropping the a)?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done (dropped the "s")

@@ -21,7 +21,7 @@

<div class="docs-api">
<h2>
API reference for Angular Material {$ doc.name $}
API reference for Angular {$ doc.packageDisplayName $} {$ doc.displayName $}
</h2>

<p class="docs-api-module-import">
Copy link
Member

Choose a reason for hiding this comment

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

We should fix the import statement here as well. We can just use doc.packageName for the imports package name.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

@@ -0,0 +1,90 @@
The `a11y` package provides a number of tools to improve accessibility, described below.
Copy link
Contributor

Choose a reason for hiding this comment

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

FocusMonitor as well

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll add that in a follow-up PR

* Create a `@ViewChildren` query for the options being managed.
* Initialize the `ListKeyManager`, passing in the options.
* Forward keyboard events from the managed component to the `ListKeyManager`

Copy link
Member

Choose a reason for hiding this comment

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

Add . at end of sentence?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

* supported.
*/

// The InteractivityChecker leans heavily on the ally.js accessibility utilities.
Copy link
Member

Choose a reason for hiding this comment

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

a11y.ts instead of .js?

Copy link
Member

Choose a reason for hiding this comment

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

With ally.js is a library meant, not a source file inside of Material. https://allyjs.io/

The built-in breakpoints based on [Google's Material Design
specification](https://material.io/guidelines/layout/responsive-ui.html#responsive-ui-breakpoints).
The available values are:
* Handset
Copy link
Member

Choose a reason for hiding this comment

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

Should these be styled as code? i.e. Handset

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it's fine without the code style



### MediaMatcher
`MediaMatcher` is a lower-level utility that wraps the native `matcheMedia`. This service normalizes
Copy link
Member

Choose a reason for hiding this comment

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

matchMedia, extra e

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

attach(overlayRef: OverlayRef): void {
this._overlayRef = overlayRef;
this._pane = overlayRef.overlayElement;
this._resizeSubscription.unsubscribe();
this._resizeSubscription = this._viewportRuler.change().subscribe(() => this.apply());
}

/** Performs any cleanup after the element is destroyed. */
/** Disposes all resources used by the posiiton strategy. */
Copy link
Member

Choose a reason for hiding this comment

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

position

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

constructor(
/** The position used as a result of this change. */
public connectionPair: ConnectionPositionPair,
/** @docs-private */
Copy link
Member

Choose a reason for hiding this comment

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

If this is @docs-private should it be _scrollableViewProperties

Copy link
Member Author

Choose a reason for hiding this comment

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

Can't rename b/c someone in google is using it

@@ -12,5 +12,5 @@ import {Directive, TemplateRef} from '@angular/core';
selector: '[cdkStepLabel]',
})
export class CdkStepLabel {
constructor(public template: TemplateRef<any>) { }
constructor(/** @docs-private */ public template: TemplateRef<any>) { }
Copy link
Member

Choose a reason for hiding this comment

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

_template?

Copy link
Member Author

Choose a reason for hiding this comment

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

Avoiding rename in case someone is using it

}
```

#### Wrapping
Copy link
Member

Choose a reason for hiding this comment

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

Add a short paragraph like this for typeahead?

Copy link
Member Author

Choose a reason for hiding this comment

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

Can add in a follow-up PR.

```

#### Types of key managers
There are two varieties of `ListKeyManager`: `FocusKeyManager` and `ActiveDescendantKeyManager`.
Copy link
Member

Choose a reason for hiding this comment

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

Comma instead of colon after the ListKeyManager.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

There are two varieties of `ListKeyManager`: `FocusKeyManager` and `ActiveDescendantKeyManager`.

##### FocusKeyManager
Used when options will directly recieve browser focus. Each item managed must implement the
Copy link
Member

Choose a reason for hiding this comment

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

recieve -> receive

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

##### ActiveDescendantKeyManager
Used when options will be marked as active via `aria-activedescendant`.
Each item managed must implement the
`FocusableOption` interface:
Copy link
Member

Choose a reason for hiding this comment

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

This should be Highlightable. Also might be worth noting that each item requires an id and that id should be bound to the host.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

readonly value: Direction = 'ltr';

/** Steam that emits whenever the 'ltr' / 'rtl' state changes. */
Copy link
Member

Choose a reason for hiding this comment

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

Steam -> Stream

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

/**
* Whether one or more media queries match the current viewport size.
* @param value One or more media queries to check.
* @returns Whether any of the media queries matches.
Copy link
Member

Choose a reason for hiding this comment

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

matches -> matched

Copy link
Member Author

Choose a reason for hiding this comment

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

Done ("match")

@@ -0,0 +1,59 @@
CDK stepper provides a foundation upon which more concrete stepper varities can be built upon. A
Copy link
Member

Choose a reason for hiding this comment

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

The sentence has one too many "upon"-s.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done


### Behavior captured by CdkStepper
The base CDK version of the stepper primarily manages which step is active. This includes handling
keyboard interactions and exposing an API for advancing or rewinding through th workflow.
Copy link
Member

Choose a reason for hiding this comment

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

th -> the

Copy link
Member Author

Choose a reason for hiding this comment

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

Done


#### Linear stepper
A stepper marked as `linear` requires the user to complete previous steps before proceeding
to following steps. For each step, the `stepControl` attribute can be set to the top level
Copy link
Member

Choose a reason for hiding this comment

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

I think the to following steps is redundant.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

### Types of steps

#### Optional step
If completion of a step in linear stepper is not required, then the `optional` attribute can be set
Copy link
Member

Choose a reason for hiding this comment

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

...in a linear stepper

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

@jelbourn jelbourn added the P1 Impacts a large percentage of users; if a workaround exists it is partial or overly painful label Oct 25, 2017
* Add JsDocs where missing
* Adds and polishes overviews for main cdk subpackages
* Tweaks dgeni to disambiguate between material and cdk output (e.g. with "table").
* Tweaks markdown process to disambiguate between material and cdk output
* Fix an issue where method return types were missing from _all_ api docs
* Correct some `any` property types in cdk/stepper
* Correct table trackBy JsDoc being omitted because it was on the setter
Copy link
Member

@crisbeto crisbeto left a comment

Choose a reason for hiding this comment

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

LGTM

@crisbeto crisbeto added pr: lgtm action: merge The PR is ready for merge by the caretaker and removed pr: needs review labels Oct 26, 2017
@mmalerba mmalerba merged commit 0f2c272 into angular:master Oct 27, 2017
@jelbourn jelbourn deleted the cdk-docs branch April 2, 2018 22:30
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 8, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker cla: yes PR author has agreed to Google's Contributor License Agreement docs This issue is related to documentation P1 Impacts a large percentage of users; if a workaround exists it is partial or overly painful
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants