-
Notifications
You must be signed in to change notification settings - Fork 27.4k
Conversation
`Which directives support animations?` section: Added info about ngAnimateSwap directive. Sorted table alphabetically, now it's the same order as appear in documentation's navigation. References angular#16561
Synced this table with one which appears in animations guide (docs/content/guide/animations.ngdoc). References angular#16561
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here (e.g. What to do if you already signed the CLAIndividual signers
Corporate signers
|
1 similar comment
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here (e.g. What to do if you already signed the CLAIndividual signers
Corporate signers
|
I signed it! |
CLAs look good, thanks! |
1 similar comment
CLAs look good, thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM except for the (re-)ordering. My guess is that you tried to switch to some sort of alphabetic ordering, but it doesn't seem to have worked out very well 😛
Yup, I've reordered it, so they appear in the same order as in navigation of the docs, what's bad about it? (So, it's sorted by module names, then by component names). |
Oh, I see. This is not at all obvious to the reader (especially since module names are not shown in the table.
Previously, they were grouped by animation type (e.g. structural vs non-structural), but not ordered other than that (afaict). Also, similar directives where close to each other (e.g. Please either order alphabetically by directive name (either ungrouped or grouped by animation type) or leave previous order. |
Ah, I get it now. Well, then I'll put them alphabetically since it's the only obvious (for reader) way of handling it I guess (the structural naming is not so openly described in docs). |
Reordered directives tables due to pull request's review (angular#16581) Refers angular#16561 Refers angular#16581
Tables reordered & unified. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM (with one minor suggestion) 👍
src/ngAnimate/module.js
Outdated
* (More information can be found by visiting the documentation associated with each directive.) | ||
* | ||
* For a full breakdown of the steps involved during each animation event, refer to the | ||
* {@link ng.$animate API docs}. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The text of the link will API docs
(which is kind of vague). Maybe change it to `$animate` API docs
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, just copied & pasted it from animations guide, wait for the commit ⌛️
Reorder directives tables due to pull request's review (angular#16581) Unify under-the-table informations. Refers angular#16561 Refers angular#16581
6153ca4
to
b652568
Compare
Done! |
What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
Updated animaton-aware directive tables both on ngAnimate's module and on animate guide pages.
What is the current behavior? (You can also link to an open issue here)
That's an effect of discussion which took place under #16561
What is the new behavior (if this is a feature change)?
No, just docs.
Does this PR introduce a breaking change?
No.
Please check if the PR fulfills these requirements
Other information: