fix(tree): errors following a full review of the component - #1946
Draft
lskramarov wants to merge 8 commits into
Draft
fix(tree): errors following a full review of the component#1946lskramarov wants to merge 8 commits into
lskramarov wants to merge 8 commits into
Conversation
🚨 E2E tests failedReview the report for details. 💡 Comment |
|
Visit the preview URL for this PR (updated for commit 3f94c35): https://koobiq-next--prs-1946-tmsv6fhb.web.app (expires Fri, 04 Sep 2026 16:15:38 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: c9e37e518febda70d0317d07e8ceb35ac43c534c |
lskramarov
force-pushed
the
review/tree
branch
from
August 26, 2026 17:46
0e09056 to
414de00
Compare
🚨 E2E tests failedReview the report for details. 💡 Comment |
Defining --kbq-tree-size-toggle-padding turned on two padding declarations that had never resolved, widening .kbq-tree-node-toggle from 16px to 24px. The 24px that KbqTreeNodePadding.iconWidth reserves for a node without a toggle stands in for the 16px toggle plus the option's 8px gap, so every row with a chevron ended up 8px to the right of its leaf siblings. The contract is now written down next to iconWidth. Pointing .kbq-option-text at --kbq-tree-size-text-padding-vertical fixed the reference but changed the value from 0 to 2px, growing every two-line row by 4px and dropping its text below the chevron and checkbox beside it, whose margins are tuned to a text box with no padding of its own.
Documenting KbqTreeNodePadding.iconWidth drops its `// (undocumented)` marker from the report. Content taken verbatim from the diff API Extractor printed on CI.
The tree-signals schematic shipped without a section in the upgrade guide, and this is the one review whose unmigrated call sites throw rather than fail to compile: assigning to a getter-only property is a TypeError in any strict-mode build.
`feat(list,tree,core)!: allow changing multiple after initialization` builds the model as `SelectionModel<SelectionModelOption>` and swaps the instance when the multiplicity changes, so the review's widening back to `any` is dropped along with its argument that no call site produces that shape — and the `any` ratchet prefers the typed one either way.
lskramarov
force-pushed
the
review/tree
branch
from
September 1, 2026 15:49
a53460a to
3e663b1
Compare
The snapshot came from upstream while the rebase ran, so it carried none of the review's narrowings. Regenerated from a fresh build rather than edited by hand.
lskramarov
force-pushed
the
review/tree
branch
from
September 1, 2026 15:50
3e663b1 to
88ec30a
Compare
🚨 E2E tests failedReview the report for details. 💡 Comment |
Two consequences of following main's `SelectionModel<SelectionModelOption>` instead of the review's widening. The spec reads a selected node back as the consumer's own type, which the declared shape does not describe, so the call site casts and says why. And the `any` ratchet had to record the improvement it produced: tree drops from 41 to 40.
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
1 participant
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.
Summary
The tree is a hard fork of
@angular/cdk/treethat re-implements the whole CDK tree concept set, andit carries the debt of an old fork: a fragile global static node handoff, an 811-line god component,
pervasive
any, and a half-finished signals migration. The review closed the accessibility gap,finished the migration behind an
ng updateschematic, and added the unit coverage several of itssub-modules never had.
The second commit fixes a deprecated binding in the docs and a stale invariant comment.
List of notable changes:
which gains a locale-backed accessible name (
optionActionsinKbqA11yLocaleConfiguration)tree-selection.component.ts, so the tree only blurs when the optionthat lost focus is the one the key manager considers active
tree-signalsschematicfor the renamed surface
flat-tree-control.filters,flat-data-source,padding.directiveandtoggleKBQ_SELECTION_TREE_VALUE_ACCESSOR,KBQ_TREE_OPTION_PARENT_COMPONENT,getParents,nodeDefs, thedataSource/indent/recursiveaccessor pairs andindentUnitsallchanged shape or left the report
(onSelectAll)output, which this same branch marks@deprecated … will be removed in version 20in favour ofselectAllChange@letread covers all fourreads of
selectAllState, which stopped being true when this branch removed two of them; the row'sown
aria-checkedhost binding is a third, independent readWhat should reviewers focus on?
packages/schematics/src/migrations/tree-signals/and its registration.packages/components/core/option/action.tsandcore/utils/dom.tsare shared;selectandtree-selectalso touch this area in this series.packages/components/tree-select/tree-select.component.tsis modified here as a consumer of thechanged tree contract, and the
tree-selectbranch modifies the same file.selectAllTargetswalk would needa new public input on
KbqTreeOption, and the extra walk produces no wrong output.One of ten pull requests from a single review pass (
review/*). Every branch carries two commits: thefixes from the component review, and a second pass over the findings an independent verifier could not
confirm outright.
The branches share files, so they have to be merged one at a time with a rebase in between. Overlap:
packages/components/core/locales/*(7 branches),tools/public_api_guard/components/core.api.md(7),tools/cspell-locales/*.json(6),packages/e2e/routes.ts(4) andpackages/schematics/src/{collection,migrations}.json(3).