feat: build the footer row lazily when createFooterRow is enabled at runtime - #1269
Merged
Conversation
…runtime
Footer-row DOM was built only in the init path and internal_setOptions
never created it, so setOptions({ createFooterRow: true }) on a live grid
flowed into setColumns -> createColumnFooter, dereferenced the undefined
_footerRowL and threw - leaving the grid with a half-mutated options
state.
internal_setOptions now materializes the footer DOM when the flag flips
true: materializeFooterRow() mirrors the init construction (R-before-L
scroller order, spacers, columns containers, hide when !showFooterRow)
and binds the footer contextmenu/click/scroll handlers on an
already-initialized grid. It runs before setScroller (which selects the
footer scroll container) and before setColumns (which populates footer
cells), so the normal pipeline completes the job. Disabling
createFooterRow at runtime hides the footer rather than destroying it,
symmetric with showFooterRow; setFooterRowVisibility works on the
runtime-built footer as on an init-built one.
Adds a permanent SELF-HOSTING regression test (cypress/e2e/quirk-runtime-
footer-enable.cy.ts): enable on a live grid -> no throw, scrollers
visible, one footer cell and one onFooterRowCellRendered per column,
getFooterRow() returns the element, setFooterRowVisibility(false) hides.
Verified to fail pre-fix (TypeError: Cannot read properties of undefined)
and pass with the fix; footer + composite-editor + frozen suites ran
27/27 as a regression gate.
NOTE: examples/example-quirk-runtime-footer-enable.html is a TEMPORARY
human-review repro page intended to be deleted before merge - the cypress
test does not depend on it.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Collaborator
|
replicated on my side and all my tests passed, so it seems valid and ready to merge |
The init path's inline footer-row construction is replaced by a call to materializeFooterRow(), making it the single construction path so the two cannot drift. The initialized guard keeps event binding unchanged: during init the handlers are still bound in finishInitialization; on a live grid the method binds them itself. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Owner
|
Actually, I thought it was a bit much repeating all of the footer materialising code for the after init case. |
Collaborator
|
ok that's a bit weird that in that case it ended up with even more loc after the refactor. A quick look at it, looks ok though |
Owner
|
From what I can see the refactor dropped 17 lines from slick.grid.ts (+51 -20 after vs +48 before). May have increased the tests LOC? Anyway, wiil merge! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The bug (Q2 in discussion #1247)
Footer-row DOM was built only in the init path, and
internal_setOptionsnever created it — sosetOptions({ createFooterRow: true })on a live grid flowed intosetColumns→createColumnFooter, dereferenced the undefined_footerRowL, and threw, leaving the grid with a half-mutated options state.The change (the 'support it' resolution from the triage)
internal_setOptionsnow materializes the footer DOM when the flag flips true.materializeFooterRow()is the single construction path shared with init — the init path's inline footer block is replaced by a call to the same method (R-before-L scroller order, spacers, columns containers, hidden when!showFooterRow), so the two paths cannot drift. On an already-initialized grid it also binds the footer contextmenu/click/scroll handlers; during init those stay bound infinishInitialization, exactly as before. In the runtime path it runs beforesetScroller(which selects the footer scroll container) and beforesetColumns(which populates footer cells), so the existing pipeline completes the job with no further special-casing.Runtime disable hides the footer rather than destroying it — symmetric with
showFooterRow— andsetFooterRowVisibilityworks on a runtime-built footer exactly as on an init-built one.Test
cypress/e2e/quirk-runtime-footer-enable.cy.ts— self-hosting (harness viacy.intercept; no example-page dependency). Enable on a live grid → no throw, scrollers visible, one footer cell and oneonFooterRowCellRenderedper column,getFooterRow()returns the element,setFooterRowVisibility(false)hides it. Verified to fail pre-fix (TypeError: Cannot read properties of undefined) and pass with the fix; footer + composite-editor + frozen suites ran 27/27 as a regression gate.One review note: an earlier draft asserted
setOptions({ showFooterRow: false })hides the footer — dropped because master never supported visibility toggling viasetOptionsfor init-built footers either;setFooterRowVisibilityis the supported API and is what the spec exercises.examples/example-quirk-runtime-footer-enable.htmlis a human-review repro (enable + toggle buttons with a live readout) intended to be deleted before merge — the cypress test is fully independent of it.(Quirks-triage remaining-items wave: see discussion #1247; siblings #1266, #1267, #1268.)