Skip to content

fix(NcActions): handle expensive height computation by popover library#7778

Merged
Antreesy merged 1 commit intomainfrom
fix/noid/nc-actions-performance
Nov 14, 2025
Merged

fix(NcActions): handle expensive height computation by popover library#7778
Antreesy merged 1 commit intomainfrom
fix/noid/nc-actions-performance

Conversation

@Antreesy
Copy link
Contributor

@Antreesy Antreesy commented Nov 5, 2025

☑️ Resolves

  • Replaces fix(NcActions): Use full window height #5806
    • autoBoundaryMaxSize lets floating vue resize the popper inner container to the available size
    • drawback - losing feature of showing 50% of last item cropped, if it doesn't fit the screen
    • benefits - no more jumping and second re-render (first by library, then manually by us), no memory leaks

🖼️ Screenshots

🏚️ Before

2025-11-05_11h22_00.mp4

🏡 After

2025-11-05_11h17_43.mp4

🚧 Tasks

  • Check effect on Memory/Performance
  • Check for side effects

🏁 Checklist

  • ⛑️ Tests are included or are not applicable
  • 📘 Component documentation has been extended, updated or is not applicable
  • 2️⃣ Backport to stable8 for maintained Vue 2 version or not applicable

@Antreesy Antreesy added this to the 9.2.0 milestone Nov 5, 2025
@Antreesy Antreesy self-assigned this Nov 5, 2025
@Antreesy Antreesy added 3. to review Waiting for reviews feature: actions Related to the actions components technical debt labels Nov 5, 2025
@codecov
Copy link

codecov bot commented Nov 5, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 51.75%. Comparing base (82d7687) to head (b95e84b).
⚠️ Report is 41 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #7778      +/-   ##
==========================================
+ Coverage   51.41%   51.75%   +0.34%     
==========================================
  Files          96       96              
  Lines        3147     3126      -21     
  Branches      867      863       -4     
==========================================
  Hits         1618     1618              
+ Misses       1279     1262      -17     
+ Partials      250      246       -4     

☔ 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.

@Antreesy Antreesy force-pushed the fix/noid/nc-actions-performance branch from 9456615 to 42a6e84 Compare November 5, 2025 11:21
@susnux

This comment was marked as resolved.

@Antreesy

This comment was marked as resolved.

@susnux

This comment was marked as resolved.

@susnux
Copy link
Contributor

susnux commented Nov 5, 2025

So while it now takes full height, it looses the main reason for the manual resizing:
If the content is longer than the boundary then we want to have it cut off at 50% of the element so the user knows about that there is more to scroll.
With only the library it does no longer cut off at 50% of the next element.

So codewise I am fine, but just needs approval of @jancborchardt (added that feature).

@DorraJaouad

This comment was marked as resolved.

@Antreesy
Copy link
Contributor Author

Antreesy commented Nov 5, 2025

so the user knows about that there is more to scroll.

Drawback would be then only for browsers, who don't show the scrollbar by default, right?

- `autoBoundaryMaxSize` lets floating vue resize the popper inner container to the available size

Signed-off-by: Maksim Sukharev <[email protected]>
@Antreesy Antreesy force-pushed the fix/noid/nc-actions-performance branch from 42a6e84 to b95e84b Compare November 7, 2025 15:30
Copy link
Contributor

@jancborchardt jancborchardt left a comment

Choose a reason for hiding this comment

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

Really nice improvement so I’d approve. :) We can check later if there is adjustment needed for cutting of at 1/2 of entries, or if that’s an issue at all (probably depending on system and scrollbar visibility).

@Antreesy Antreesy requested a review from susnux November 14, 2025 12:33
@susnux
Copy link
Contributor

susnux commented Nov 14, 2025

Isnt this solved by #7808 ?

@Antreesy
Copy link
Contributor Author

Isnt this solved?

Performance - mostly solved.
But it still triggers re-rendering of a popover after mounting, it looks jumpy and frozen, if trigger button is scrolled (up and down). So I would still like to opt in for native library solution

@ShGKme ShGKme modified the milestones: 9.2.0, 9.3.0 Nov 14, 2025
Copy link
Contributor

@susnux susnux left a comment

Choose a reason for hiding this comment

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

code makes sense and so as design agrees this is fine :)

@Antreesy Antreesy merged commit 4acc10d into main Nov 14, 2025
27 checks passed
@Antreesy Antreesy deleted the fix/noid/nc-actions-performance branch November 14, 2025 20:11
@ShGKme
Copy link
Contributor

ShGKme commented Nov 14, 2025

@Antreesy backport?

@Antreesy
Copy link
Contributor Author

/backport to stable8

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

3. to review Waiting for reviews feature: actions Related to the actions components technical debt

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants