feat(design): expand selected tree items on initial render #2938
Conversation
|
@xelaint can you review the behavior to make sure that you're happy with it? |
|
I'm not sure if I like the scroll to view behavior. Can we remove that for now? |
griest024
force-pushed
the
feat/design/tree/expand-active-items-on-initial-render
branch
from
971595f
to
a68a5b1
Compare
okay removed for now but the user can easily end up in a position where they cannot see the active item on initial render |
|
@xelaint note that the focusing behavior of tree item actually appears to scroll the element into view. This doesn't work for deeply nested items though ( |
| * Opens parent and parent of parent all the way to the root of the tree. | ||
| */ | ||
| openAncestors() { | ||
| const openParent = (tree: DaffTreeUi<unknown>) => { |
There was a problem hiding this comment.
Remove the recursion and replace with iteration.
There was a problem hiding this comment.
what list would be iterated? Recursion is naturally suited to tree structures
There was a problem hiding this comment.
I updated this to use collect (which is itself recursive but has the added protection of a max depth) and iteration.
| this.items.changes.pipe( | ||
| takeUntilDestroyed(this.destroyRef), | ||
| ).subscribe((items: QueryList<DaffTreeItemDirective>) => { | ||
| runInInjectionContext(this.injector, () => afterNextRender(() => { |
There was a problem hiding this comment.
Is the selected state only available after ngAfterViewInit? Can this be done sooner? Perhaps ngAfterContentInit?
If this needs ngAfterViewInit can we do this without a subscription?
There was a problem hiding this comment.
the ContentChildren aren't available in ngAfterContentInit because they're rendered with ngTemplateOutlet. yeah I could do it without the sub
There was a problem hiding this comment.
actually no, this doesn't work. The selected value of the tree items isn't updated
| const activeTreeItem = items.find((treeItem) => treeItem.selected); | ||
| if (activeTreeItem) { | ||
| activeTreeItem.openAncestors(); | ||
| this.cd.markForCheck(); |
There was a problem hiding this comment.
I don't believe that a markForCheck is sufficient here. It may be that we have to do a full detectChanges. I would expect Angular to error here with a Expression changed after is has been checked.
There was a problem hiding this comment.
I haven't noticed that error in the console. I can still change it if you want though
There was a problem hiding this comment.
I think the task queue shift created by the observable is likely hiding the error.
|
The UI LGTM. |
|
I haven't re-reviewed this yet, but please do not merge it until I do. |
griest024
force-pushed
the
feat/design/tree/expand-active-items-on-initial-render
branch
from
58acbec
to
8563015
Compare
|
bump @damienwebdev |
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Fixes: #2931
Does this PR introduce a breaking change?
Other information