Conversation
|
@mjbvz @jrieken hi I found two question:
|
|
Sorry I just saw the ci error, I will fix it |
| dispose(): void { } | ||
| abstract equals(other: BreadcrumbsItem): boolean; | ||
| abstract render(container: HTMLElement): void; | ||
| abstract supportDrag(): boolean; |
There was a problem hiding this comment.
An optional property might be better than a method, e.g supportsDrag?: boolean
| container.classList.add('monaco-breadcrumb-item'); | ||
| if (item.supportDrag()) { | ||
| container.draggable = true; | ||
| this._disposables.add(dom.addStandardDisposableListener(container, 'dragstart', e => this._onCrumbDrag(e))); |
There was a problem hiding this comment.
This is not OK. Everytime an item is rendered (which happens repeatedly) a listener is added but not disposed unless the breadcrumbs widget is disposed. The listeners must be managed with the lifecycle of the _renderItem-call.
Because of this and to avoid having too many listeners I recommended doing this:
- only set draggable here
- have only a single drag listener for the root dom node
- have logic in the single listener to figure out what item is being dragged
| } | ||
|
|
||
| const index = this._nodes.indexOf(event.target as HTMLDivElement); | ||
| const payload = event as any; |
There was a problem hiding this comment.
Not sure I like this. Maybe have a special event type for DND and be more defensive, e.g return early when index is < 0 etc
| } | ||
|
|
||
| private _willStartDragEvent(event: IBreadcrumbsItemEvent): void { | ||
| if (event.item instanceof FileItem === false) { |
There was a problem hiding this comment.
Should this ever happen given how event is set up?
| } | ||
|
|
||
| supportDrag(): boolean { | ||
| return false; |
There was a problem hiding this comment.
I believe outline items should also support DND. They can use the file-uri and the postion anchor, like file:///some/path.txt#L12:34 for line 12, column 34 of file some/path.txt
|
@jrieken I'm sorry, due to some reasons, I haven't been able to continue coding in the past few months. May I kindly ask if it's possible for me to resume work on this pull request? If so, I would be happy to reorganize the code based on your suggestions. |
|
Any progress? |
4 participants
Fix #182685