Skip to content
This repository has been archived by the owner on Jan 13, 2025. It is now read-only.

docs(animation): Simplify README to follow best practices #1062

Merged
merged 3 commits into from
Aug 2, 2017

Conversation

lynnmercier
Copy link
Contributor

No description provided.

@lynnmercier lynnmercier requested a review from acdvorak August 2, 2017 17:43
@codecov-io
Copy link

codecov-io commented Aug 2, 2017

Codecov Report

Merging #1062 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1062   +/-   ##
=======================================
  Coverage   99.93%   99.93%           
=======================================
  Files          68       68           
  Lines        3295     3295           
  Branches      405      405           
=======================================
  Hits         3293     3293           
  Misses          2        2

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 339cdd0...f4cc68a. Read the comment docs.

Copy link
Contributor

@acdvorak acdvorak left a comment

Choose a reason for hiding this comment

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

Looks good overall, just a few small clarifications :-)

We currently have variables for the following 4 animation curves:
### CSS Classes

Some components have a set typographic style. For example, a raised MDC Checkbox enter and exit curves.
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm a little confused by this line. Could you possibly clarify a bit?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ooo, ya i copy pasted badly. Updated.


Some components have a set typographic style. For example, a raised MDC Checkbox enter and exit curves.

If you want to animate an element, which is not a Material Design component, you can apply the following CSS classes.
Copy link
Contributor

Choose a reason for hiding this comment

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

element, which -> element that

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

| `$mdc-animation-sharp-curve-timing-function` | `cubic-bezier(.4, 0, .6, 1)` | Animations that cause objects to leave the screen temporarily (e.g. closing a drawer) |
CSS Class | Description
--- | ---
`mdc-animation-deceleration-curve` | Sets the speed curve to deceleration
Copy link
Contributor

Choose a reason for hiding this comment

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

The purpose of these classes might be clearer if we reword the descriptions to something like:

Sets `animation-timing-function` to the deceleration curve

Or, alternatively, we could add a sentence above the table that explains that each of these classes sets the animation-timing-function to a particular speed curve.

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 don't want to put animation-timing-function in the description...because it is an implementation detail. Is there any other way to make this more clear without just documenting the output of the CSS class?

property:
Mixin | Description
--- | ---
`mdc-animation-deceleration-curve` | Sets the speed curve to deceleration
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto my above comment about CSS class descriptions


```scss
@import "@material/animation/functions";
Copy link
Contributor

Choose a reason for hiding this comment

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

It might be easier for newcomers if we keep the @import declarations (and add them to the examples that lack them) so they don't have to spend time figuring which path to import.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

| `getCorrectPropertyName(windowObj: Object, eventType: string)` | Returns a CSS property name. Prefixed if necessary. |
Method Signature | Description
--- | ---
`getCorrectEventName(windowObj, eventType)` | Returns a JavaScript event name, prefixed if necessary |
Copy link
Contributor

Choose a reason for hiding this comment

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

Trailing | can be removed (ditto below)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Copy link
Contributor

@acdvorak acdvorak left a comment

Choose a reason for hiding this comment

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

LGTM!

@lynnmercier lynnmercier merged commit 933a41a into master Aug 2, 2017
@lynnmercier lynnmercier deleted the docs/animation branch August 17, 2017 00:14
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants