-
Notifications
You must be signed in to change notification settings - Fork 29.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Proposals for Collapse, CollapseAll, Expand APIs #61626
Conversation
export function createTreeView<T>(viewId: string, options: { treeDataProvider: TreeDataProvider<T> }): TreeView2<T>; | ||
} | ||
|
||
export interface TreeView2<T> extends TreeView<T> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You seem to like the 2
suffix but I think that's not really needed...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried using the same name
export interface TreeView2<T> extends TreeView<T> { | |
export interface TreeView<T> extends TreeView<T> { |
but it caused recursive compilation error. Do you have other suggestions?
/** | ||
* Collapse all elements. | ||
*/ | ||
collapseAll(): Thenable<void>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe just another overload, like collapse()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tried this but felt weird collapse
does collapsing all.
* @param elementOrElements element(s) to collpase. | ||
* @param recursive Controls if elements have to be collapsed recursively or not. | ||
*/ | ||
collapse(elementOrElements: T | T[], recursive?: boolean): Thenable<void>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why Thenable
? Is this us leaking tree implementation details?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
expand
is async and to be consistent I made collapse
also. In fact collapse
is async in the current tree impl. Not returning Thenable
might cause authors to think it sync and can lead to unexpected errors.
@sandy081 You can just merge proposed api changes, we will discuss them in the API call, and lets have a PR when merging this to stable |
Implements #55879