-
Notifications
You must be signed in to change notification settings - Fork 30.4k
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
Adopt new tree in custom views #76407
Conversation
Note: This is still missing auto expanding of nodes, a feature that doesn't yet exist in the new tree. Once the functionality is available I'll add it before merging. |
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.
@alexr00 Changes look good and there are some minor suggestions. Please take a look. Thanks.
const resourceLabel = this.labels.create(container, { supportHighlights: true, donotSupportOcticons: true }); | ||
DOM.addClass(resourceLabel.element, 'custom-view-tree-node-item-resourceLabel'); | ||
const actionsContainer = DOM.append(resourceLabel.element, DOM.$('.actions')); | ||
const actionsContainer = DOM.append(container, DOM.$('.actions')); |
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.
@alexr00 One minor question as asked before, is this change intentional? Why not use the same structure as before?
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.
It is needed to have the correct alignment of the actions. Appending to the resourceLabel.element
causes the action buttons to be right up against the resource label, instead of nicely aligned to the right.
@alexr00 I had just one last comment otherwise LGTM |
Auto expand is still missing. I'm merging now to start getting selfhost bugs. |
Fixes #63566