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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Animate directive #12067

Merged
merged 8 commits into from
Nov 8, 2022
Merged

Animate directive #12067

merged 8 commits into from
Nov 8, 2022

Conversation

cetincakiroglu
Copy link
Contributor

@cetincakiroglu cetincakiroglu commented Oct 20, 2022

Add pAnimate directive to handle animations on a scroll by adding/removing PrimeFlex Animation classes.

@cetincakiroglu cetincakiroglu linked an issue Oct 20, 2022 that may be closed by this pull request
@github-actions
Copy link

👋 Hi,
Thanks a lot for your contribution! But, PR does not seem to be linked to any issues. Please manually link to an issue or mention it in the description using #<issue_id>.

@github-actions github-actions bot added the Resolution: Needs Revision The pull request can't be merged. Conflicts need to be corrected or documentation and code updated. label Oct 20, 2022
}
}

ngOnDestroy() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Hi, you also need to unsubscribe from IntersecrtionObserver(use method unobserve)

this.observer.observe(this.host.nativeElement);
}

isVisible(element) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this code should run out of ngzone (since IntersectionObserver can throw events frequently).

this.observer.observe(this.host.nativeElement);
}

isVisible(element) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add types here?


bindDocumentScrollListener() {
if (!this.documentScrollListener) {
this.documentScrollListener = this.renderer.listen(window, 'scroll', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think all listeners for scroll|load also should invoke outside zone

const [intersectionObserverEntry] = element;
this.entered = intersectionObserverEntry.isIntersecting;

if (this.entered || this.isInViewport()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you should call this method once and save(the result) it in a variable, because using the getBoundingClientRect method causes reflow in browser

Copy link
Contributor

Choose a reason for hiding this comment

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

it's about isInViewport


animate() {
if (this.loaded) {
if (this.isInViewport() && !this.entered) {
Copy link
Contributor

Choose a reason for hiding this comment

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

the same store variable from isInViewport

@github-actions
Copy link

👋 Hi,
Thanks a lot for your contribution! But, PR does not seem to be linked to any issues. Please manually link to an issue or mention it in the description using #<issue_id>.

@github-actions
Copy link

github-actions bot commented Nov 8, 2022

👋 Hi,
Thanks a lot for your contribution! But, PR does not seem to be linked to any issues. Please manually link to an issue or mention it in the description using #<issue_id>.

@github-actions
Copy link

github-actions bot commented Nov 8, 2022

👋 Hi,
Thanks a lot for your contribution! But, PR does not seem to be linked to any issues. Please manually link to an issue or mention it in the description using #<issue_id>.

2 similar comments
@github-actions
Copy link

github-actions bot commented Nov 8, 2022

👋 Hi,
Thanks a lot for your contribution! But, PR does not seem to be linked to any issues. Please manually link to an issue or mention it in the description using #<issue_id>.

@github-actions
Copy link

github-actions bot commented Nov 8, 2022

👋 Hi,
Thanks a lot for your contribution! But, PR does not seem to be linked to any issues. Please manually link to an issue or mention it in the description using #<issue_id>.

@cetincakiroglu cetincakiroglu removed the Resolution: Needs Revision The pull request can't be merged. Conflicts need to be corrected or documentation and code updated. label Nov 8, 2022
@cetincakiroglu cetincakiroglu merged commit 954104d into master Nov 8, 2022
@cetincakiroglu cetincakiroglu deleted the issue-12057 branch November 8, 2022 12:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

pAnimate Directive
2 participants