Skip to content

fix(module:icon): debounce icon rendering on animation frame - #8579

Merged
Laffery merged 1 commit into
NG-ZORRO:masterfrom
arturovt:fix/7825
Jun 27, 2025
Merged

fix(module:icon): debounce icon rendering on animation frame#8579
Laffery merged 1 commit into
NG-ZORRO:masterfrom
arturovt:fix/7825

Conversation

@arturovt

@arturovt arturovt commented Jun 14, 2024

Copy link
Copy Markdown
Member

Closes #7825

@arturovt arturovt changed the title fix(module:icon): debounce icon rendering on animatin frame fix(module:icon): debounce icon rendering on animation frame Jun 14, 2024
@zorro-bot

zorro-bot Bot commented Jun 14, 2024

Copy link
Copy Markdown

This preview will be available after the AzureCI is passed.

@codecov

codecov Bot commented Jun 14, 2024

Copy link
Copy Markdown

Codecov Report

Attention: Patch coverage is 92.85714% with 1 line in your changes missing coverage. Please review.

Project coverage is 92.06%. Comparing base (bf2b33b) to head (615edbf).
Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
components/icon/icon.directive.ts 92.85% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #8579      +/-   ##
==========================================
- Coverage   92.06%   92.06%   -0.01%     
==========================================
  Files         560      560              
  Lines       19621    19625       +4     
  Branches     3098     3099       +1     
==========================================
+ Hits        18064    18067       +3     
  Misses       1218     1218              
- Partials      339      340       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@arturovt
arturovt marked this pull request as ready for review June 16, 2024 14:43
@arturovt
arturovt requested a review from wzhudev as a code owner November 7, 2024 11:40
Comment thread components/icon/icon.directive.ts Outdated

@HyperLife1119 HyperLife1119 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull Request Overview

This PR debounces icon rendering on animation frames to improve performance when many icons are rendered, integrates PendingTasks for SSR hydration, and centralizes zone handling.

  • Debounce icon rendering with animationFrameScheduler on browser and asapScheduler on server.
  • Inject and use PendingTasks to register hydration tasks and finalize them correctly.
  • Move zone boundaries: invoke changeIcon2 outside Angular in ngOnChanges.
Comments suppressed due to low confidence (4)

components/icon/icon.directive.ts:133

  • There are no tests covering the new debounce behavior in browser vs. server environments. Please add unit tests to verify debouncing works as intended and tasks are finalized correctly.
      debounceTime(0, this.isBrowser ? animationFrameScheduler : asapScheduler),

components/icon/icon.directive.ts:127

  • [nitpick] The name removeTask is vague. Consider renaming to something like removePendingTask to clarify that this callback unregisters a pending hydration task.
    const removeTask = this.pendingTasks.add();

components/icon/icon.directive.ts:139

  • [nitpick] The parameter svgOrRemove is ambiguous. Rename it to svgElement or renderedSvg for clarity on what the callback receives.
      next: svgOrRemove => {

components/icon/icon.directive.ts:122

  • The method changeIcon2 assumes it runs outside Angular but only ngOnChanges wraps it in runOutsideAngular. If changeIcon2 is called elsewhere, it might trigger unwanted change detection. Consider moving runOutsideAngular inside changeIcon2 for consistency.
  private changeIcon2(): void {

@Laffery Laffery left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM

@Laffery

Laffery commented Jun 27, 2025

Copy link
Copy Markdown
Collaborator

@arturovt please rebase to resolve conflicts

@Laffery
Laffery merged commit c0709d1 into NG-ZORRO:master Jun 27, 2025
9 of 10 checks passed
@arturovt
arturovt deleted the fix/7825 branch June 27, 2025 10:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

icons rendering performance too slow.

4 participants