HTM-1812: TOC in mobile layout #1122
Conversation
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
...cts/core/src/lib/components/toc/toc-node-details-mobile/toc-node-details-mobile.component.ts
Fixed
Show fixed
Hide fixed
...cts/core/src/lib/components/toc/toc-node-details-mobile/toc-node-details-mobile.component.ts
Fixed
Show fixed
Hide fixed
...cts/core/src/lib/components/toc/toc-node-details-mobile/toc-node-details-mobile.component.ts
Fixed
Show fixed
Hide fixed
# Conflicts: # projects/core/src/lib/components/edit/edit/edit.component.html
There was a problem hiding this comment.
Pull request overview
This pull request implements Table of Contents (TOC) functionality for mobile layouts by adding the ability to display layer information inline within the tree view. The changes introduce a new mobile-specific component for displaying layer details and modify the tree component to support this expanded view.
Changes:
- Added
TocNodeDetailsMobileComponentto display layer info inline in the tree for mobile devices - Enhanced tree component with new inputs to support info display within tree nodes
- Added mobile TOC mode that shows layer info within the tree instead of in a separate panel
Reviewed changes
Copilot reviewed 18 out of 19 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| projects/shared/src/lib/components/tree/tree.component.ts | Added inputs for mobile info display: openInfoInTree, nodeInfoInTreeTemplate, and activeInTreeInfoNodes; disabled selection when openInfoInTree is true |
| projects/shared/src/lib/components/tree/tree.component.html | Added conditional CSS classes and template rendering for in-tree info display |
| projects/shared/src/lib/components/tree/tree.component.css | Added styling for selected nodes with info display |
| projects/core/src/lib/map/models/index.ts | Exported layer-tree-node-with-layer.model |
| projects/core/src/lib/layout/mobile-layout/mobile-layout.component.html | Passed mobileToc flag to TOC component |
| projects/core/src/lib/components/toolbar/simple-search/simple-search.component.ts | Removed console.debug statement |
| projects/core/src/lib/components/toc/toc/toc.component.ts | Added mobileToc input, activeMobileInfoNodes state, mobile panel height setting, and setShowMobileInfo method |
| projects/core/src/lib/components/toc/toc/toc.component.html | Added mobile info template, conditional rendering for mobile mode, and passed mobile-specific props to tree |
| projects/core/src/lib/components/toc/toc/toc.component.css | Added mobile-specific padding styles |
| projects/core/src/lib/components/toc/toc.module.ts | Declared and exported TocNodeDetailsMobileComponent |
| projects/core/src/lib/components/toc/toc-node-layer/toc-node-layer.component.ts | Added showInfoIcon input and emitShowInfo output |
| projects/core/src/lib/components/toc/toc-node-layer/toc-node-layer.component.html | Added info icon button for mobile view |
| projects/core/src/lib/components/toc/toc-node-layer/toc-node-layer.component.css | Added flexbox layout for wrapping mobile info display |
| projects/core/src/lib/components/toc/toc-node-details/toc-node-details.component.html | Fixed indentation |
| projects/core/src/lib/components/toc/toc-node-details-mobile/* | New component files for mobile layer details display |
| projects/core/src/lib/components/toc/index.ts | Exported TocNodeDetailsMobileComponent |
| @Input() | ||
| public openInfoInTree = false; | ||
|
|
||
| @Input() | ||
| public nodeInfoInTreeTemplate?: TemplateRef<any>; | ||
|
|
||
| @Input() | ||
| public activeInTreeInfoNodes: string[] = []; |
There was a problem hiding this comment.
The new openInfoInTree, nodeInfoInTreeTemplate, and activeInTreeInfoNodes inputs lack test coverage. The shared tree component is widely used, and these new inputs modify selection and rendering behavior. Consider adding tests to verify that these inputs work correctly, especially the conditional selection behavior when openInfoInTree is true.
| public editLayer = new EventEmitter<string>(); | ||
|
|
||
| @Output() | ||
| public emitShowInfo= new EventEmitter<boolean>(); |
There was a problem hiding this comment.
Missing space after 'emitShowInfo'. The property name should have a space before the equals sign to match the formatting of other EventEmitter declarations in this file (see lines 39 and 42).
| public emitShowInfo= new EventEmitter<boolean>(); | |
| public emitShowInfo = new EventEmitter<boolean>(); |
| public editLayer = new EventEmitter<string>(); | ||
|
|
||
| @Output() | ||
| public emitShowInfo= new EventEmitter<boolean>(); |
There was a problem hiding this comment.
The EventEmitter emits a boolean value but the value is not used in the handler. The handler 'setShowMobileInfo' takes a FlatTreeModel node parameter, not a boolean. Consider changing the EventEmitter type to EventEmitter<void> or EventEmitter<FlatTreeModel> if you intend to pass the node through the event.
| [editableLayerIds]="getCurrentlyEditableLayerIds()" | ||
| [showInfoIcon]="mobileToc()" | ||
| (zoomToScale)="zoomToScale($event)" | ||
| (editLayer)="editLayer($event)" |
There was a problem hiding this comment.
The event handler receives 'node' directly from the template context, but the EventEmitter emits 'true'. This mismatch means the handler will receive 'node' from the template's let-node binding, not from the event emission. This works but is confusing. Consider either: 1) Emitting the node from showInfo() and updating the EventEmitter type, or 2) Documenting that the node comes from the template context, not the event.
| (editLayer)="editLayer($event)" | |
| (editLayer)="editLayer($event)" | |
| <!-- emitShowInfo emits a boolean; setShowMobileInfo intentionally uses the node from the template context --> |
| public showInfo() { | ||
| this.emitShowInfo.emit(true); | ||
| } |
There was a problem hiding this comment.
The new showInfo() method and emitShowInfo EventEmitter lack test coverage. The existing test file has tests for other component functionality but does not cover the new mobile info icon behavior. Consider adding tests to verify that clicking the info icon properly emits the event.
| public setShowMobileInfo(node: FlatTreeModel) { | ||
| const activeMobileInfoNodes = this.activeMobileInfoNodes(); | ||
| if (activeMobileInfoNodes.includes(node.id)) { | ||
| this.activeMobileInfoNodes.set(activeMobileInfoNodes.filter(id => id !== node.id)); | ||
| } else { | ||
| this.activeMobileInfoNodes.set([ ...activeMobileInfoNodes, node.id ]); | ||
| } | ||
| } |
There was a problem hiding this comment.
The new setShowMobileInfo() method lacks test coverage. The method manages the activeMobileInfoNodes signal, which is critical for showing/hiding layer info in mobile view. Consider adding tests to verify that this method correctly toggles node IDs in the activeMobileInfoNodes array.
2 participants
Uh oh!
There was an error while loading. Please reload this page.