Skip to content
This repository was archived by the owner on Apr 12, 2024. It is now read-only.

Fixes for animate docs #16581

Closed
wants to merge 3 commits into from
Closed

Conversation

FRSgit
Copy link
Contributor

@FRSgit FRSgit commented May 27, 2018

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

  • The commit message follows our guidelines
  • Fix/Feature: Docs have been added/updated
  • Fix/Feature: Tests have been added; existing tests pass

Other information:

FRSgit added 2 commits May 28, 2018 00:19
`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
@googlebot
Copy link

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. I signed it!) and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

1 similar comment
@googlebot
Copy link

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. I signed it!) and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

@FRSgit
Copy link
Contributor Author

FRSgit commented May 27, 2018

I signed it!

@googlebot
Copy link

CLAs look good, thanks!

1 similar comment
@googlebot
Copy link

CLAs look good, thanks!

Copy link
Member

@gkalpak gkalpak left a 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 😛

@FRSgit
Copy link
Contributor Author

FRSgit commented May 28, 2018

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).
Did the previous order have any sense at all? It wasn't done by type (modules/directives), not by name, even parts of the same modules wasn't always near each other (like ngMessages & ngMessage).

@gkalpak
Copy link
Member

gkalpak commented May 28, 2018

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.

Did the previous order have any sense at all?

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. ngHide/ngShow).

Please either order alphabetically by directive name (either ungrouped or grouped by animation type) or leave previous order.

@FRSgit
Copy link
Contributor Author

FRSgit commented May 28, 2018

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. ngHide/ngShow).

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).
Still, the main focus there was to sync table from naAnimate page & animation guide.

@gkalpak gkalpak self-assigned this May 28, 2018
FRSgit added a commit to FRSgit/angular.js that referenced this pull request Jun 1, 2018
Reordered directives tables due to pull request's review (angular#16581)

Refers angular#16561
Refers angular#16581
@FRSgit
Copy link
Contributor Author

FRSgit commented Jun 1, 2018

Tables reordered & unified.

Copy link
Member

@gkalpak gkalpak left a 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) 👍

* (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}.
Copy link
Member

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?

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, 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
@FRSgit FRSgit force-pushed the fixes-for-animate-docs branch from 6153ca4 to b652568 Compare June 1, 2018 12:34
@FRSgit
Copy link
Contributor Author

FRSgit commented Jun 1, 2018

Done!

@gkalpak gkalpak closed this in 586b6e8 Jun 2, 2018
gkalpak pushed a commit that referenced this pull request Jun 2, 2018
- Synced "animation aware" directives tables in API docs and "Animation"
  guide.
- Sorted tables alphabetically.
- Added info about `ngAnimateSwap` directive.

References #16561

Closes #16581
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants