feat(ui): decouple tree nodes from the UI representation #2783
Conversation
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
JadenSimon
commented
Jul 28, 2022
| * Any new or existing code needs to account for this additional layer as the shim | ||
| * would be passed in as-is. | ||
| */ | ||
| export class TreeShim extends AWSTreeNodeBase { |
There was a problem hiding this comment.
The shim was updated to support this new functionality. It's not a perfect recreation because TreeNode can handle async rendering of the node itself while the shim would show Loading... on initialization.
| id: 'placeholder', | ||
| resource: message, | ||
| treeItem: new vscode.TreeItem(message, vscode.TreeItemCollapsibleState.None), | ||
| getTreeItem: () => new vscode.TreeItem(message, vscode.TreeItemCollapsibleState.None), |
There was a problem hiding this comment.
I don't like turning otherwise constant things into functions but it was kind of necessary for this functionality.
It makes things harder to test and reason about :/
| const recordExpandCdkOnce = once(telemetry.recordCdkAppExpanded) | ||
| view.onDidExpandElement(e => { | ||
| if (e.element.resource instanceof AppNode) { | ||
| if (e.element.resource instanceof CdkRootNode) { |
There was a problem hiding this comment.
Metric was being recorded when the child node expanded rather than the parent (my bad).
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.
Problem
While prototyping features, I kept running into the situation where I wanted to update how a node was rendered independently of the node's children. The current implementation does not easily support this use-case, forcing us to propagate events up to a parent node to get the desired behavior.
Solution
Fully generalize the
TreeNodeinterface. It was already very close to the general solution, it just needed a few tweaks to get it there. Of course, implementing this correctly with a tree data provider required more substantial changes which tests were added for.I wouldn't recommend that all tree node implementations use this pattern because it can quickly spiral out of control from a maintainability point of view. Still, I'd rather have the proper solution readily available than a bunch of one-off bespoke solutions.
License
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.