refactor: render grid header and footer row parts declaratively#12225
Conversation
vaadin-review-bot
left a comment
vaadin-review-bot
left a comment
There was a problem hiding this comment.
Reviewed the changes — left 2 comments and collected 1 finding below that could not be attached to the diff.
| Finding | |
|---|---|
| 👀 | Footer slot-name level numbering flipped, but only grid's snapshot was regenerated — the crud snapshot still has the old numbers. |
| ⚡ | The column tree is sorted twice per header/footer render instead of once. |
| 🧹 | #headerRows and #footerRows are near-identical getters differing only by the visibility function and .toReversed(). |
👀 Footer slot-name level numbering flipped, but only grid's snapshot was regenerated — the crud snapshot still has the old numbers.
The crud DOM snapshot embeds a grid with a 2-level column tree, so its footer content nodes carry slot names at both levels: vaadin-grid-footer-cell-content-1-1, 1-3, 1-4, 0-0, 0-2, 0-4. Because #footerRows now calls .toReversed() before the level-assigning .map(), the level passed to #renderFooterRow is the reversed index, so these footer slot names swap 0↔1 — exactly the swap the PR already applied to grid's own snapshot (grid.test.snap.js lines ~594-651).
Only grid's snapshot was regenerated, so yarn test:snapshots --group crud will fail. Regenerate the crud snapshot (yarn update:snapshots --group crud). This is the only other package whose snapshots embed a multi-level grid footer.
crud.test.snap.js:191 · test-coverage · confirmed
cf99cb8 to
d440d7b
Compare
|
/code-review |
There was a problem hiding this comment.
DOM snapshots changed because directive-rendered part attributes contain extra whitespace on initial render. Lit's classMap produces the same whitespace for class, but the snapshot library normalizes the class attribute and keeps part as is.
d440d7b to
e7ea1c2
Compare
|
/code-review |
vaadin-review-bot
left a comment
vaadin-review-bot
left a comment
There was a problem hiding this comment.
Reviewed the changes — left 3 comments.
| Finding | |
|---|---|
| 🧹 | #getHeaderRows and #getFooterRows are near-duplicate methods |
| 🧹 | findIndex/findLastIndex re-scan the rows array once per row inside the .map |
| ⚡ | cellParts is rebuilt for every cell though it is constant per row |
c96d1e4 to
672c3e3
Compare
faadebc to
c8f2f39
Compare
|
/code-review |
vaadin-review-bot
left a comment
vaadin-review-bot
left a comment
There was a problem hiding this comment.
Reviewed the changes — left 2 comments.
| Finding | |
|---|---|
| ⚡ | _swapColumnOrders iterates all body rows twice instead of once. |
| 🧹 | Redundant literal space before partMap() bakes double/trailing spaces into the part attribute. |
c8f2f39 to
15f9331
Compare
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Declarative rendering no longer leaves trailing spaces in the row part attribute values. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
15f9331 to
0cdfe57
Compare
700cf40 to
aa5bd98
Compare
|
3 participants
This PR continues the migration of the imperatively applied header/footer row and cell state into Lit templates. It covers:
first-header-row,last-header-row,first-footer-row,last-footer-rowrow parts with their-cellvariants and matching classesfirst-column/last-columnattributes andfirst-column-cell/last-column-cellparts with matching classes on header and footer cellsPart of #10789