-
-
Notifications
You must be signed in to change notification settings - Fork 266
[POC] Detached triggers for Dialog #2283
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
Conversation
Bundle size report
|
✅ Deploy Preview for base-ui ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Does this API also solve #2157? |
|
Just as an thought, the keyword e.g. This would only apply if |
|
The latest version doesn't use any new parts (except for Dialog.Provider) - only Dialog.Root and Dialog.Trigger are used. If a Dialog.Trigger has a |
commit: |
|
interesting poc! are you planning to make a detached trigger reactive to the open state? |
|
Yes, I'm going to. I'm also going to change the implementation significantly based on my learnings from #2336. |
|
Implemented in #2974 |
Proof of concept for Dialog's detached triggers.
Ordinary triggers: https://deploy-preview-2283--base-ui.netlify.app/experiments/dialog/perf-ordinary-triggers
Detached triggers: https://deploy-preview-2283--base-ui.netlify.app/experiments/dialog/perf-detached-triggers
Usage: see docs/src/app/(private)/experiments/dialog/perf-detached-triggers.tsx
The idea of a "detached" trigger is to place it outside of Dialog.Root. Such a trigger references a target dialog by its id (we might allow refs as well). This helps to avoid rendering many Dialog.Root components in scenarios like data lists with actions (as shown in the demos). Many triggers can reuse a single Dialog instance providing different data to it.
This requires an extra Dialog.Provider component that sets up a global context so relation between dialogs and triggers can be established.
This implementation assumes the usage of DetachedTrigger is optional. Dialogs still work the same way using the existing Dialog.Trigger API (without requiring Dialog.Provider).
Detached triggers show significant perf improvements.
The charts display less scripting work needed for detached triggers. Results vary from run to run, but the trend is clear - for the example with 200 rows, scripting time after the
loadevent takes about 2x more time with ordinary triggers (~300ms vs ~600ms on 6x CPU slowdown). Also memory usage is significantly lower (peaking at ~15MB for detached and ~24MB for ordinary triggers). This is mostly due to reducing the number of rendered React nodes and contexts.cc @romgrk