refactor: Chart.js implementation in chart widgets#19681
refactor: Chart.js implementation in chart widgets#19681ahmed-rashad-alnaggar wants to merge 10 commits intofilamentphp:4.xfrom
Conversation
Signed-off-by: ahmed-rashad-alnaggar <131385452+ahmed-rashad-alnaggar@users.noreply.github.com>
Signed-off-by: ahmed-rashad-alnaggar <131385452+ahmed-rashad-alnaggar@users.noreply.github.com>
Signed-off-by: ahmed-rashad-alnaggar <131385452+ahmed-rashad-alnaggar@users.noreply.github.com>
Upcoming PhasesTo keep pull requests concise and maintainable, the following updates will be handled in separate stages:
Note: This staged approach ensures each PR remains focused and specific, as requested. |
There was a problem hiding this comment.
Thanks. I think I need to see where the rest of this is going before I can judge this phase confidently on its own.
The part I’m most concerned about is the manual dataset/state diff/state-sync path. I understand it is being introduced to support in-place updates, but if this approach is going to work, that part needs to be extremely robust. It is also the same area that caused issues in the previous PR, and a big reason that PR felt too broad to me.
Since this is already being presented in phases, this intermediate step is hard to evaluate in isolation without seeing the intended end state more clearly.
If you continue with the rest of the plan, it would help a lot if you could include the test scenarios you’re validating, along with a few short videos. That would make the review much easier.
|
Just to confirm I understood correctly — we’re good to proceed with this phase, and for the upcoming update/state-sync part you’d like me to include test scenarios and short videos demonstrating the behavior, right? |
|
Yes, you can continue and submit the full implementation. But it’s worth noting that, if the full approach still relies on the same manual dataset/state diff/state-sync shadow layer as before, then I would still consider it a broad and high-risk change. If that layer is not clearly proven stable first, I don’t think we’re even at the point of meaningfully validating whether moving to update()/resize() avoids regressions. Put simply, I think this approach can only move forward if you can show that the frontend state is being maintained correctly and does not introduce new issues. We should not introduce instability just to improve performance or visual behavior. |
Signed-off-by: ahmed-rashad-alnaggar <131385452+ahmed-rashad-alnaggar@users.noreply.github.com>
Signed-off-by: ahmed-rashad-alnaggar <131385452+ahmed-rashad-alnaggar@users.noreply.github.com>
Signed-off-by: ahmed-rashad-alnaggar <131385452+ahmed-rashad-alnaggar@users.noreply.github.com>
|
This is Phase 2 of 4. Building on the refactor in Phase 1, this PR implements dynamic chart resizing — eliminating the destroy-and-reinitialize cycle that previously occurred on every resize event. What's Changed
Screen Recording
Beforefilament-chart-resize-old.webmAfterfilament-chart-resize-new.webm |
Added user-defined color options for chart customization. Signed-off-by: ahmed-rashad-alnaggar <131385452+ahmed-rashad-alnaggar@users.noreply.github.com>
Signed-off-by: ahmed-rashad-alnaggar <131385452+ahmed-rashad-alnaggar@users.noreply.github.com>
|
This is Phase 3 of 4. Building on the in-place resize introduced in Phase 2, this PR eliminates the remaining destroy-and-reinitialize cycle — the one that fired on every theme change. What's Changed
Screen Recordingfilament-chart-theme-change.webm |
|
I think it would make sense for this to be the final phase in this PR. The remaining work in Phase 4 (updating/syncing chart data with reactive and animation logic) feels like a separate concern from the refactor and lifecycle improvements covered in Phases 1–3. It might be cleaner to handle that in a dedicated follow-up PR, so this one stays focused and easier to review. |
This is Phase 1 of 4.
Adhering to the feedback from @People-Sea comment, this PR refactors the Chart widget's Chart.js implementation. This is a pure refactor; no functional changes or new features are introduced (except animation).
What’s Changed
Removed Global Defaults Mutation: Stopped the practice of overriding global
Chart.defaults(e.g.backgroundColor,borderColor,color,font.family) insideinitChart(). Visual fallbacks (colors, fonts, etc.) are now applied locally to the per-chartoptionsobject instead. The two genuinely global defaults (legend.labels.boxWidthandlegend.position) are hoisted to module level, where they belong.Removed Animation Duration Default: The line
Chart.defaults.animation.duration = 0was removed.Cleaner Event Handling: Moved logic for theme changes, data updates, and resizing into dedicated methods (
updateChartTheme,updateChartData,resizeChart) instead of bulky inline anonymous functions.initChartrenamed to_initChart()and itsdataparameter removed: The leadingunderscore signals that this is an internal method not intended to be called directly.
The
data = nullparameter was also dropped —_initChartnow always reads fromcachedDatadirectly, sinceupdateChartDatahandles live data updates independently.Helper-Driven Architecture:
whenChart()to centralize null-safety checks before interacting with the Chart.js instance, eliminating three near-identical guard blocks that were duplicated across event handlers.getComputedStylecalls intogetChartFallbackColors()for better readability and easier testing.Decoupled Aspect Ratio Logic:
options.maintainAspectRatiois now derived by directly checkingthis.$refs.canvas.style.maxHeight !== ''instead of relying on themaxHeightprop passed from outside. This decouples the logic from external property drilling and removes an unnecessary function parameter.Minor —
destroy()uses optional chaining: Replaced the manualif (this.resizeObserver)guard withthis.resizeObserver?.disconnect().