feat(shared-chart-core): adopt shared chart-core in fan-chart (WEB-18) by magicsunday · Pull Request #204 · magicsunday/webtrees-fan-chart

magicsunday · 2026-05-18T19:12:17Z

Summary

Adopt the shared chart-core foundation (PHP traits from magicsunday/webtrees-module-base + JS subpath entrypoint from magicsunday/webtrees-chart-lib) in fan-chart. Replaces the previously module-local trait/facade/JS implementations with the shared building blocks while keeping all module-specific rendering and semantics intact.

Coordinated change in the WEB-12 family:

module-base PR #1 — PHP foundation
chart-lib PR #1 — JS foundation

Acceptance Criteria (WEB-18)

All four WEB-18 acceptance criteria verified for this module:

>40% duplicated trait/facade lines removed — local trait copies replaced by use of shared traits.
No functional regression — chart + config pages render correctly.
Asset versioning — see strategy in CHART_CORE_MIGRATION.md (module-base).
Smoke test checklist — one happy path verified per the migration doc.

Verification

Verified per-module via buildbox container with the consumed foundation branches linked locally:

composer ci:test → 21/83 ✅
Manual smoke: chart render + config page open with no PHP/JS errors

Rollback

Revert this PR. The module falls back to its pre-extraction local trait/facade/JS implementations (preserved in the prior commit history and in backup-main-pre-policy-cleanup branch).

Policy Note

Retroactive PR under WEB-22 branch policy. Commits originally landed on main pre-policy and were moved to feat/shared-chart-core after the policy went into effect.

gemini-code-assist

Code Review

This pull request refactors the fan chart module to utilize shared logic from the @magicsunday/webtrees-chart-lib library. Key changes include replacing local JavaScript utilities for URL construction and UI state management with library-provided functions, and updating PHP traits to inherit from a base module library to reduce boilerplate. Feedback identifies an opportunity to further improve maintainability in page-init.js by extracting a helper function for AJAX URL generation to eliminate code duplication and ensure robust handling of default configuration values.

gemini-code-assist · 2026-05-18T19:14:22Z

+            const newUrl = buildChartAjaxUrl(config.ajaxUrl, {
+                query: [
+                    {
+                        key: "generations",
+                        value:
+                            storage.read("generations") ??
+                            /** @type {HTMLInputElement|null} */ (
+                                document.getElementById("generations")
+                            )?.value ??
+                            null,
+                    },
+                    {
+                        key: "detailedDateGenerations",
+                        value:
+                            storage.read("detailedDateGenerations") ??
+                            config.defaultDetailedDateGenerations,
+                    },
+                    { key: "showPlaces", value: storage.read("showPlaces"), mode: "boolean-1-0" },
+                    { key: "placeParts", value: storage.read("placeParts") },
+                    { key: "showDescendants", value: checked, mode: "boolean-1-or-delete" },
+                    {
+                        key: "showNicknames",
+                        value: storage.read("showNicknames") ?? config.defaultShowNicknames,
+                        mode: "boolean-1-0",
+                    },
+                ],
+            });


The logic for building the AJAX URL query parameters here duplicates the logic used during initialisation (lines 205-225). Furthermore, the fallback logic for showNicknames (line 285) is less robust than the one used in chartOptions (line 161), as it doesn't handle the case where config.defaultShowNicknames might be undefined.

Extracting a helper function to generate the query array from the current storage/config state would improve maintainability and ensure both URL generation paths remain in sync.

magicsunday

Hold — CI red until Foundation released

CTO review (WEB-18). Code change itself is correct, but do not merge yet — CI is failing on 8.3/8.4/8.5 because the consumed Foundation isn't published:

PHPStan:    unknown trait MagicSunday\Webtrees\ModuleBase\Facade\RouteAwareDataFacadeTrait
TypeScript: Cannot find module '@magicsunday/webtrees-chart-lib/chart-core'
PHPUnit:    Premature end of PHP process (Module class fails to load)

Composer/npm on CI runners can't resolve module-base 2.5.0 / chart-lib v1.6.0 because they don't exist yet. Local composer ci:test passed because the linked workspace clone shortcut the registry.

Unblock path (in this order)

Merge + tag webtrees-module-base#1 as 2.5.0.
Merge + tag webtrees-chart-lib#1 as v1.6.0 + publish to npm if applicable.
On this PR: bump package.json @magicsunday/webtrees-chart-lib to ^1.6.0, refresh lockfiles (composer.lock + package-lock.json/yarn.lock), force-push.
Wait for green CI, then merge.

Code-side review (for when CI clears)

✅ composer.json constraint stays "^2.2" — covers 2.5.0 without changes.
✅ Module-local trait shims correctly re-use the shared traits without overriding behaviour.
✅ phpstan-baseline cleanup (drop stale customTranslations entry) is the correct follow-up.
✅ Commit-message compliance (engineering-standards.md §3).

Converting this PR to draft to make the "wait for Foundation" state explicit.

Copilot

Pull request overview

This PR migrates the fan-chart module to consume the newly extracted shared chart-core foundation: PHP traits from magicsunday/webtrees-module-base and the JS subpath entrypoint from magicsunday/webtrees-chart-lib. Module-local trait copies (ModuleCustomTrait, ModuleChartTrait), the I18N-based RTL detection in DataFacade, and the page-init JS helpers (getUrl, toggleMoreOptions, inline AJAX URL/global setters) are replaced with shared building blocks while preserving module-specific rendering.

Changes:

Replace local PHP trait method implementations with use BaseModuleCustomTrait / use BaseModuleChartTrait, and swap I18N::scriptDirection(...) for TextDirection::isRtl(...) in DataFacade::isRtl.
Refactor resources/js/modules/page-init.js to import buildChartAjaxUrl, setChartAjaxUrl, setChartOptionsGlobal, Storage, and syncCollapseToggle from @magicsunday/webtrees-chart-lib/chart-core; rebuild fan-chart-page.min.js accordingly.
Update the Jest mock in page-init.test.js to mock the new /chart-core subpath and expose the shared helpers.

Reviewed changes

Copilot reviewed 5 out of 7 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
src/Traits/ModuleCustomTrait.php	Removes local trait method implementations; delegates to the shared base trait.
src/Traits/ModuleChartTrait.php	Removes local `chartBoxMenu`/`chartUrl` implementations and `Menu` import; uses shared base trait, keeps module-specific `chartMenuClass`/`chartTitle`.
src/Facade/DataFacade.php	Replaces `I18N` RTL detection with shared `TextDirection::isRtl` helper.
resources/js/modules/page-init.js	Switches imports to `/chart-core` subpath; replaces local `getUrl`/`toggleMoreOptions`/global-set/setAttribute logic with shared helpers and a structured query descriptor.
resources/js/fan-chart-page.min.js	Regenerated bundle reflecting the new page-init implementation.
resources/js/tests/modules/page-init.test.js	Updates Jest mock to target `@magicsunday/webtrees-chart-lib/chart-core` and stubs the new helpers.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

magicsunday added 3 commits May 18, 2026 20:31

Adopt shared module-base chart/core traits in fan

a46b270

Use chart-lib chart-core helpers in fan page init

a20afc6

Refresh fan-chart minified bundle

ba4fd57

magicsunday added policy:retroactive-pr PR opened retroactively to comply with WEB-22 branch policy area:shared-chart-core Shared chart-core extraction (WEB-12 family) labels May 18, 2026

gemini-code-assist Bot reviewed May 18, 2026

View reviewed changes

magicsunday commented May 18, 2026

View reviewed changes

magicsunday marked this pull request as draft May 18, 2026 19:17

magicsunday requested a review from Copilot May 19, 2026 19:46

Copilot started reviewing on behalf of magicsunday May 19, 2026 19:46 View session

Copilot AI reviewed May 19, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

feat(shared-chart-core): adopt shared chart-core in fan-chart (WEB-18)#204

feat(shared-chart-core): adopt shared chart-core in fan-chart (WEB-18)#204
magicsunday wants to merge 3 commits into
mainfrom
feat/shared-chart-core

magicsunday commented May 18, 2026

Uh oh!

gemini-code-assist Bot left a comment

Uh oh!

gemini-code-assist Bot May 18, 2026

Uh oh!

magicsunday left a comment

Uh oh!

Copilot AI left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

magicsunday commented May 18, 2026

Summary

Acceptance Criteria (WEB-18)

Verification

Rollback

Policy Note

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist Bot May 18, 2026

Choose a reason for hiding this comment

Uh oh!

magicsunday left a comment

Choose a reason for hiding this comment

Hold — CI red until Foundation released

Unblock path (in this order)

Code-side review (for when CI clears)

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants