|
8 | 8 | ## Learnings |
9 | 9 |
|
10 | 10 | <!-- Append new learnings below. Each entry is something lasting about the project. --> |
| 11 | +<!-- ⚠ Summarized 2026-02-12 by Scribe — original entries covered 2026-02-10 through 2026-02-12 --> |
11 | 12 |
|
12 | | -### 2026-02-10 — PR Review & Sprint Planning Session |
| 13 | +### Summary: Milestone 1 PR Review & Planning (2026-02-10) |
13 | 14 |
|
14 | | -**PR #333 (Calendar):** |
15 | | -- Strongest of the 6 PRs. Table-based rendering matches Web Forms output. 19 tests. SelectionMode uses string instead of enum — Web Forms uses `CalendarSelectionMode` enum (None/Day/DayWeek/DayWeekMonth). Missing `CalendarSelectionMode` enum in Enums/. Style properties use CSS class strings (`TitleStyleCss`) instead of Web Forms `TableItemStyle` objects — acceptable pragmatic trade-off for Blazor. Missing: `UseAccessibleHeader` property, `Caption`/`CaptionAlign` properties. The `.GetAwaiter().GetResult()` call in `CreateDayRenderArgs` is a blocking anti-pattern but necessary for synchronous rendering. Overall quality is high. |
| 15 | +Reviewed 6 PRs. Calendar (#333): strong but needed CalendarSelectionMode enum. FileUpload (#335): broken data flow — must use InputFile, not raw @onchange. ImageMap (#337): wrong base class (BaseWebFormsComponent → should be BaseStyledComponent), static ID counter leak. PageService (#327): solid DI/service approach. #328 (ASCX CLI) and #309 (VS Snippets): merge conflicts, shelved indefinitely. |
16 | 16 |
|
17 | | -**PR #335 (FileUpload):** |
18 | | -- Inherits `BaseStyledComponent` ✓. Uses `<input type="file">` — correct HTML output. `OnFileChangeInternal` uses raw `ChangeEventArgs` instead of Blazor `InputFile`/`IBrowserFile` pattern — the `@onchange` binding won't actually populate `_currentFile`. This is a broken data flow: files will never be loaded. Has security comments from GitHub Advanced Security about `_currentFiles` readonly and `Path.Combine` traversal risk. `Accept` and `AllowMultiple` attributes correct. Missing: `HasFiles` (plural) property from Web Forms. |
| 17 | +Gate review: Calendar REJECTED (assigned Rogue), FileUpload REJECTED (assigned Jubilee, path sanitization needed), ImageMap APPROVED, PageService APPROVED. Cyclops locked out of Calendar/FileUpload. PR #333 closed — work already on dev. |
19 | 18 |
|
20 | | -**PR #337 (ImageMap):** |
21 | | -- Correctly renders `<img>` + `<map>` + `<area>` HTML structure matching Web Forms. HotSpot hierarchy (HotSpot → RectangleHotSpot/CircleHotSpot/PolygonHotSpot) matches Web Forms class hierarchy exactly. Implements `IImageComponent` interface. Uses `BaseWebFormsComponent` not `BaseStyledComponent` — this is wrong; Web Forms `ImageMap` inherits from `Image` which inherits `WebControl` which has style properties. Static `_mapIdCounter` with `Interlocked.Increment` is thread-safe but will leak across test runs. Missing: `Enabled` property handling for areas. |
| 19 | +### Summary: Milestone 2 & 3 Planning (2026-02-10–02-11) |
22 | 20 |
|
23 | | -**PR #327 (PageService):** |
24 | | -- Novel approach — not a direct Web Forms control, but emulates `Page.Title`, `Page.MetaDescription`, `Page.MetaKeywords`. Uses DI service pattern (IPageService) — idiomatic Blazor. Renders `<PageTitle>` and `<HeadContent>` — correct for Blazor 6+. Generic catch clauses flagged by code scanning. Useless variable assignments in tests flagged. Solid architectural approach for the migration use case. |
| 21 | +- Milestone 2 shipped: Localize, MultiView+View, ChangePassword, CreateUserWizard. 41/53 (77%). |
| 22 | +- Status.md reconciliation: actual count was 48/53 (91%) after accounting for merged work. |
| 23 | +- Milestone 3 scope: DetailsView + PasswordRecovery. Chart/Substitution/Xml deferred. |
| 24 | +- Colossus added as integration test engineer. |
25 | 25 |
|
26 | | -**PR #328 (ASCX CLI Tool):** |
27 | | -- Merge conflicts — NOT mergeable. Draft status. Converts `<%@ Control %>`, `<asp:*>`, `<%: %>`, `<%= %>`, `<%# %>`, `<% %>` blocks. Has `AiAssistant` stub class. No tests visible in the tool project itself. This is a companion tool, not a component — different review criteria. Needs conflict resolution and test coverage before merge. |
| 26 | +### Summary: Milestone 3 Gate Review (2026-02-11) |
28 | 27 |
|
29 | | -**PR #309 (VS Snippets):** |
30 | | -- Merge conflicts — NOT mergeable. 13 VS 2022 snippets as VSIX. Not a component — tooling review. Snippets for static imports and component patterns. Useful but needs rebase to resolve conflicts. |
| 28 | +DetailsView APPROVED: DataBoundComponent<T> inheritance, 10 events, table HTML, DetailsViewMode enum, auto-field generation. PasswordRecovery APPROVED: BaseWebFormsComponent inheritance, 3-step wizard, 6 events, table HTML. Minor non-blocking issues noted. 50/53 components (94%), 797 tests. |
31 | 29 |
|
32 | | -**Key Patterns Discovered:** |
33 | | -- Copilot-authored PRs consistently use good XML doc comments |
34 | | -- Components generally follow the project's base class hierarchy correctly |
35 | | -- Calendar uses string-based SelectionMode instead of enum — inconsistent with project enum pattern |
36 | | -- FileUpload has a fundamental data flow bug with `@onchange` not populating file data |
37 | | -- ImageMap should inherit BaseStyledComponent, not BaseWebFormsComponent |
38 | | -- Two PRs (#328, #309) have merge conflicts blocking any merge |
| 30 | +**Key patterns confirmed:** |
| 31 | +- Login Controls → BaseWebFormsComponent + cascading TableItemStyle/Style |
| 32 | +- Data-bound controls → DataBoundComponent<T> with Items parameter |
| 33 | +- Login Control events use `On` prefix (project convention) |
| 34 | +- Docs + samples must ship with components |
39 | 35 |
|
40 | | -**Sprint Planning Decisions:** |
41 | | -- Sprint 1 should focus on landing Calendar (with SelectionMode enum fix) and PageService, plus fixing merge conflicts on tooling PRs |
42 | | -- Sprint 2 should tackle remaining Editor Controls (MultiView/View, Localize) and start Login Controls |
43 | | -- Sprint 3 should cover Data Controls gaps (DetailsView) and documentation/sample catch-up |
| 36 | +### Summary: Chart JS Library Evaluation (2026-02-12) |
| 37 | + |
| 38 | +Evaluated 4 JS libraries for Chart component. D3 rejected (zero built-in charts, XL effort). Chart.js recommended (MIT, ~60KB, 10 types, Blazor wrappers exist). ApexCharts strong alternative (20+ types but 2x bundle). Plotly rejected (3-4MB). Architecture: bundle chart.min.js as static asset + thin JS interop layer. HTML output exception: `<canvas>` instead of `<img>` (justified). Inherit DataBoundComponent<T>. Effort: L. Risks: first JS interop in project, canvas not bUnit-testable, SSR needs special handling. Proceed as Milestone 4. |
44 | 39 |
|
45 | 40 | 📌 Team update (2026-02-10): PRs #328 (ASCX CLI) and #309 (VS Snippets) shelved indefinitely — decided by Jeffrey T. Fritz |
46 | | -📌 Team update (2026-02-10): Docs and samples must ship in the same sprint as the component — decided by Jeffrey T. Fritz |
47 | | -📌 Team update (2026-02-10): Sprint plan ratified — 3-sprint roadmap established — decided by Forge |
48 | | -📌 Team update (2026-02-10): Sprint 1 gate review — Calendar (#333) REJECTED (assigned Rogue), FileUpload (#335) REJECTED (assigned Jubilee), ImageMap (#337) APPROVED, PageService (#327) APPROVED — decided by Forge |
| 41 | +📌 Team update (2026-02-10): Docs and samples must ship in the same milestone as the component — decided by Jeffrey T. Fritz |
| 42 | +📌 Team update (2026-02-10): Milestone 1 gate review — Calendar (#333) REJECTED, FileUpload (#335) REJECTED, ImageMap (#337) APPROVED, PageService (#327) APPROVED — decided by Forge |
49 | 43 | 📌 Team update (2026-02-10): Lockout protocol — Cyclops locked out of Calendar and FileUpload revisions — decided by Jeffrey T. Fritz |
50 | | -📌 Team update (2026-02-10): Close PR #333 without merging — all Calendar work already on dev, PR branch has 0 unique commits — decided by Rogue |
51 | | -📌 Team update (2026-02-10): Sprint 2 complete — Localize, MultiView+View, ChangePassword, CreateUserWizard shipped with docs, samples, tests. 709 tests passing. 41/53 components done. — decided by Squad |
52 | | - |
53 | | -### 2026-02-10 — Sprint 3 Planning & Status Reconciliation |
54 | | - |
55 | | -**Status.md was significantly stale:** |
56 | | -- Calendar was merged to dev via commit d33e156 and PR #339 but still marked 🔴 Not Started |
57 | | -- FileUpload was merged via PRs #335 and #338 but still marked 🔴 Not Started |
58 | | -- Summary table said 41/53 (Editor: 20/27) but actual count of ✅ entries in the detailed section was already 23/27 for Editors (now 25/27 with Calendar + FileUpload fixed) |
59 | | -- The 27-count for Editor Controls groups MultiView and View as one logical component despite separate table rows |
60 | | -- Corrected total: 48/53 components complete (91%), 5 remaining |
61 | | - |
62 | | -**Sprint 3 scope decision:** |
63 | | -- DetailsView and PasswordRecovery are the two buildable components |
64 | | -- Chart deferred: requires SVG/Canvas rendering engine, no Blazor primitive equivalent |
65 | | -- Substitution deferred: Web Forms output caching has no Blazor architectural equivalent |
66 | | -- Xml deferred: XSLT transforms are a dead-end pattern with near-zero migration demand |
67 | | -- Post-Sprint 3 state: 50/53 (94%), library effectively feature-complete for practical migration |
68 | | - |
69 | | -**DetailsView design notes:** |
70 | | -- Must inherit BaseStyledComponent (Web Forms DetailsView → CompositeDataBoundControl → WebControl) |
71 | | -- Renders as `<table>` with one `<tr>` per field (vertical layout vs GridView's horizontal) |
72 | | -- Can reuse existing BoundField, TemplateField, CommandField, HyperLinkField, ButtonField from GridView |
73 | | -- Needs DetailsViewMode enum (ReadOnly=0, Edit=1, Insert=2) |
74 | | -- Needs 8 EventArgs classes for mode changes, CRUD operations |
75 | | - |
76 | | -**PasswordRecovery design notes:** |
77 | | -- Must inherit BaseStyledComponent |
78 | | -- 3-step wizard flow: UserName → Question → Success (same pattern as CreateUserWizard's 2-step) |
79 | | -- Can reuse existing LoginControls style sub-components (TitleTextStyle, TextBoxStyle, LabelStyle, etc.) |
80 | | -- Table-based HTML output matching ChangePassword's render pattern |
81 | | - |
82 | | -📌 Team update (2026-02-10): Sprint 3 plan ratified — DetailsView + PasswordRecovery. Chart/Substitution/Xml deferred indefinitely with migration docs. 48/53 → target 50/53. — decided by Forge |
83 | | -📌 Team update (2026-02-11): Colossus added as dedicated integration test engineer. Rogue retains bUnit unit tests. — decided by Jeffrey T. Fritz |
84 | | - |
85 | | -### 2026-02-11 — Sprint 3 Gate Review |
86 | | - |
87 | | -**DetailsView — APPROVED:** |
88 | | -- Inherits `DataBoundComponent<ItemType>` — correct for data-bound controls. Uses same `Items` property as GridView/ListView. |
89 | | -- All 10 Web Forms events implemented with correct `EventArgs` types. Pre-operation events support cancellation. |
90 | | -- `DetailsViewMode` enum (ReadOnly=0, Edit=1, Insert=2) matches Web Forms exactly. |
91 | | -- HTML output: `<table>` with one `<tr>` per field, command row with `<a>` links, nested-table numeric pager — all match Web Forms. |
92 | | -- Auto-generation via reflection correctly generates fields from `ItemType` properties. |
93 | | -- Minor issues (non-blocking): `CombinedStyle` has CellPadding/CellSpacing logic mismatch, `cellspacing` hardcoded to 0 in template, docs use `DataSource` but actual parameter is `Items`. |
94 | | -- DetailsView docs `DataSource`→`Items` fix assigned to Beast. |
95 | | - |
96 | | -**PasswordRecovery — APPROVED:** |
97 | | -- Inherits `BaseWebFormsComponent` — consistent with ChangePassword and CreateUserWizard pattern. |
98 | | -- 3-step wizard flow (UserName → Question → Success) matches Web Forms exactly. |
99 | | -- Reuses existing `LoginCancelEventArgs`, `TableItemStyle`, `Style` cascading parameter pattern from other Login Controls. |
100 | | -- `SuccessTextStyle` sub-component added following existing `UiTableItemStyle` pattern. |
101 | | -- All 6 events implemented: `OnVerifyingUser`, `OnUserLookupError`, `OnVerifyingAnswer`, `OnAnswerLookupError`, `OnSendingMail`, `OnSendMailError`. |
102 | | -- `SetQuestion()` and `SkipToSuccess()` APIs provide developer control matching Web Forms extensibility. |
103 | | -- Table-based nested HTML output matches Web Forms PasswordRecovery output. |
104 | | -- Minor issues (non-blocking): `RenderOuterTable` declared but not used, `SubmitButtonType`/`SubmitButtonImageUrl` declared but not rendered, sample uses `e.Sender` casting instead of `@ref`. |
105 | | - |
106 | | -**Key Patterns Confirmed:** |
107 | | -- Login Controls consistently inherit `BaseWebFormsComponent` (not `BaseStyledComponent`) and use cascading `TableItemStyle`/`Style` objects for styling — this is an established project convention. |
108 | | -- Data-bound controls inherit `DataBoundComponent<T>` which provides `Items` (not `DataSource`) as the primary binding parameter. |
109 | | -- Event naming in Login Controls uses `On` prefix (`OnVerifyingUser`, `OnChangingPassword`) — project convention, not Web Forms convention. |
110 | | -- Both components ship with docs, samples, and tests per Sprint 2 policy. |
111 | | - |
112 | | -**Sprint 3 Status:** |
113 | | -- 50/53 components complete (94%) |
114 | | -- 797 tests passing, 0 build errors |
115 | | -- 3 remaining (Chart, Substitution, Xml) deferred indefinitely |
116 | | -- Library is effectively feature-complete for practical Web Forms migration |
117 | | - |
118 | | -📌 Team update (2026-02-11): Sprint 3 gate review — DetailsView APPROVED, PasswordRecovery APPROVED. 50/53 complete (94%). — decided by Forge |
119 | | - |
120 | | -### 2026-02-12 — Chart Component JS Library Evaluation |
121 | | - |
122 | | -**Requested by Jeffrey T. Fritz.** Performed full architectural evaluation of using a JavaScript charting library to implement the Web Forms `System.Web.UI.DataVisualization.Charting.Chart` control (previously deferred as "very high complexity"). |
123 | | - |
124 | | -**Key findings:** |
125 | | -- Web Forms Chart renders an `<img>` tag pointing to a server-generated PNG via `ChartImg.axd` — this is architecturally impossible to replicate in Blazor (no GDI+, no server-side image handler pipeline). Any implementation must accept a HTML output deviation. |
126 | | -- The `SeriesChartType` enum has 35 chart types (Point through Pyramid). Realistic Phase 1 subset: 8 types (Column, Bar, Line, Pie, Area, Doughnut, Scatter, StackedColumn) covering 90%+ of real-world usage. |
127 | | -- Chart's key sub-objects: Series, DataPoint, ChartArea, Axis, Legend, Title — must be mirrored as Blazor sub-components. |
128 | | - |
129 | | -**Library evaluation results:** |
130 | | -- **D3.js — REJECTED.** Low-level SVG manipulation toolkit, not a charting library. Zero built-in chart types. No Blazor wrapper. Would require building an entire charting engine from scratch (XL effort). Jeffrey's suggestion, but wrong abstraction level. |
131 | | -- **Chart.js — RECOMMENDED.** MIT license, ~60KB gzipped, ~10 built-in chart types covering all Phase 1 needs, multiple Blazor wrappers exist (though we recommend bundling chart.min.js directly and writing thin interop, not depending on community wrapper NuGet packages). |
132 | | -- **ApexCharts — STRONG ALTERNATIVE.** MIT, ~120KB gz, 20+ chart types, official Blazor wrapper (Blazor-ApexCharts). Better coverage but double the bundle size and the wrapper has its own opinionated API. |
133 | | -- **Plotly.js — REJECTED.** 3-4MB gzipped bundle size is disqualifying. Scientific/3D features are overkill. |
134 | | - |
135 | | -**Architecture recommendation:** |
136 | | -- Use Chart.js bundled as a static asset (~60KB) |
137 | | -- Thin JS interop layer (`chart-interop.js`) translating C# config → Chart.js config |
138 | | -- Blazor Chart component mirrors Web Forms property names (Series, ChartAreas, etc.) |
139 | | -- `<canvas>` output instead of `<img>` — documented as justified exception |
140 | | -- Inherit `DataBoundComponent<T>` (consistent with Web Forms Chart inheriting DataBoundControl) |
141 | | -- Effort: L (Large) — first component requiring JS interop, new testing challenges (canvas can't be bUnit-tested) |
142 | | - |
143 | | -**Risks identified:** |
144 | | -1. JS interop is unprecedented in this project — new build/packaging complexity |
145 | | -2. Canvas content not testable via bUnit — Playwright becomes primary quality gate |
146 | | -3. Chart.js version pinning required — major version upgrades could break interop |
147 | | -4. SSR/prerendering needs special handling (canvas requires DOM) |
148 | | - |
149 | | -**Recommendation: Proceed, but as Sprint 4** — not tacked onto Sprint 3. The JS interop pattern warrants dedicated design review and planning. |
150 | | - |
151 | | -📌 Team update (2026-02-12): Chart component feasibility confirmed — Chart.js recommended via JS interop. HTML output deviation (`<canvas>` not `<img>`) justified and documented. Effort: L. Target Sprint 4. — decided by Forge |
| 44 | +📌 Team update (2026-02-10): Close PR #333 without merging — all Calendar work already on dev — decided by Rogue |
| 45 | +📌 Team update (2026-02-10): Milestone 2 complete — 4 components shipped. 709 tests. 41/53 done. — decided by Squad |
| 46 | +📌 Team update (2026-02-10): Milestone 3 plan ratified — DetailsView + PasswordRecovery. Chart/Substitution/Xml deferred. — decided by Forge |
| 47 | +📌 Team update (2026-02-11): Colossus added as dedicated integration test engineer. — decided by Jeffrey T. Fritz |
| 48 | +📌 Team update (2026-02-11): Milestone 3 gate review — DetailsView APPROVED, PasswordRecovery APPROVED. 50/53 (94%). — decided by Forge |
| 49 | +📌 Team update (2026-02-12): Chart component feasibility confirmed — Chart.js recommended via JS interop. Effort: L. Target Milestone 4. — decided by Forge |
| 50 | +📌 Team update (2026-02-12): Milestone 4 planned — Chart component with Chart.js via JS interop. 8 work items, design review required before implementation. — decided by Forge + Squad |
0 commit comments