Skip to content
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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

docs(animations): improve animation transition api docs #44396

Conversation

dario-piotrowicz
Copy link
Contributor

improve the transition api docs by removing unnecessary examplanations
and examples

also provide helpful information regarding entering and leaving
elements (as part of #44253)

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • angular.io application / infrastructure changes
  • Other... Please describe:

Issue

Relates to Issue #44253

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

I started looking into these docs because as part of #44253 I wanted to add the information about entering and leaving elements, but I thought that the docs here were quite unorganised and confusing so I tried to clean them up

before there were information all over the place and many thing were repeated and/or not explained clearly in my opinion

I hope I did well, if you could please give this a preview you can see that in the way I've changed it, it is much leaner, clear and easy to follow (at least for me 馃槣)

@pullapprove pullapprove bot requested a review from jelbourn December 7, 2021 22:57
@AndrewKushnir AndrewKushnir requested review from jessicajaniuk and removed request for jelbourn December 8, 2021 00:24
@AndrewKushnir AndrewKushnir added action: review The PR is still awaiting reviews from at least one requested reviewer area: animations comp: docs target: patch This PR is targeted for the next patch release labels Dec 8, 2021
@ngbot ngbot bot modified the milestone: Backlog Dec 8, 2021
Copy link
Contributor

@jessicajaniuk jessicajaniuk left a comment

Choose a reason for hiding this comment

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

@dario-piotrowicz This looks really good. Can you add a couple of examples based on the comments I left? I think they'd be really helpful. Otherwise this looks great.

packages/animations/src/animation_metadata.ts Outdated Show resolved Hide resolved
packages/animations/src/animation_metadata.ts Outdated Show resolved Hide resolved
packages/animations/src/animation_metadata.ts Show resolved Hide resolved
packages/animations/src/animation_metadata.ts Show resolved Hide resolved
@dario-piotrowicz
Copy link
Contributor Author

@jessicajaniuk thanks a lot for the nice reviewing 馃槂 , I've added examples for the State Change Expressions please let me know what you think 馃檪

Also what do you think, should I also add an example for the function variant? I would and could be a nice addition, but I am afraid that it would look a bit weird/unclear there in the first list 馃

@jessicajaniuk
Copy link
Contributor

@dario-piotrowicz You could add just one example at the end of all of the examples and clarify in a comment that it shows what a function variant of these would look like.

Copy link
Contributor

@jessicajaniuk jessicajaniuk left a comment

Choose a reason for hiding this comment

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

LGTM 馃崻

Once you add those last little changes, this looks great! Thanks for your work!

improve the transition api docs by removing unnecessary examplanations
and examples

also provide helpful information regarding entering and leaving
elements (as part of angular#44253)
@dario-piotrowicz
Copy link
Contributor Author

@jessicajaniuk I've added an example of the function variant at the end of the page, please have a look and let me know what you think 馃檪


@dario-piotrowicz You could add just one example at the end of all of the examples and clarify in a comment that it shows what a function variant of these would look like.

I'm not sure I full understood this comment 馃槗 , are you suggesting to show the function variant for each single example I provided? I'm afraid that could be quite verbose and repetitive, wouldn't it? 馃槗
Besides I'd imagine that the function variant shouldn't be used if the string one is enough, so I am not completely sure if we should encourage that 馃 (like in my added example I am showing a sort of situation in which a user could actually want to use the function variant)

@jessicajaniuk
Copy link
Contributor

I think you've covered it. Thanks, @dario-piotrowicz. :)

@jessicajaniuk jessicajaniuk added action: merge The PR is ready for merge by the caretaker and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Dec 9, 2021
@alxhub
Copy link
Member

alxhub commented Dec 10, 2021

This PR was merged into the repository by commit 9249506.

alxhub pushed a commit that referenced this pull request Dec 10, 2021
improve the transition api docs by removing unnecessary examplanations
and examples

also provide helpful information regarding entering and leaving
elements (as part of #44253)

PR Close #44396
@alxhub alxhub closed this in 9249506 Dec 10, 2021
dimakuba pushed a commit to dimakuba/angular that referenced this pull request Dec 28, 2021
improve the transition api docs by removing unnecessary examplanations
and examples

also provide helpful information regarding entering and leaving
elements (as part of angular#44253)

PR Close angular#44396
@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 Jan 10, 2022
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 area: animations target: patch This PR is targeted for the next patch release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants