Support multiple comment widgets per notebook cell.#226770
Conversation
|
Hi there, any ETA for reviewing this? I see there is a conflict which I think has to do with a larger refactoring import style - do you need me to resolve that or are you doing that as you review and merge? Thanks for your time! |
Fixes microsoft#226769. CommentStore already allows multiple comments per cell. This change only removes the restriction to only render the first. The comments are rendered one below the next.
8b64774 to
f7d74e6
Compare
|
|
||
| override updateInternalLayoutNow(element: ICellViewModel): void { | ||
| if (this.currentElement && this._commentThreadWidget.value) { | ||
| if (this.currentElement) { |
There was a problem hiding this comment.
Does the length of this._commentThreadWidgets need to be checked here?
There was a problem hiding this comment.
Thanks for checking.
I am not exactly sure what the original intention was here.
When there is no widget, nothing would be rendered, and it does not matter where the empty container is positioned.
Maybe the goal was to avoid an assignment to style.top which may trigger a layout or something like this?
I think it would also be fine to check that this._commentThreadWidget is not empty and otherwise leave the empty container whereever it was before. Maybe that is more in line with the original logic.
Let me know if you'd like me to do that.
There was a problem hiding this comment.
Let's leave it as is. @rebornix, if you know of a reason we need to make the check then we can add it later.
alexr00
left a comment
There was a problem hiding this comment.
Thanks for the ping! I resolved the conflict from our ESM work. I have one comment, but this looks good to me.
|
|
||
| override updateInternalLayoutNow(element: ICellViewModel): void { | ||
| if (this.currentElement && this._commentThreadWidget.value) { | ||
| if (this.currentElement) { |
There was a problem hiding this comment.
Let's leave it as is. @rebornix, if you know of a reason we need to make the check then we can add it later.
Fixes #226769.
CommentStore already allows multiple comments per cell. This change only removes the restriction to only render the first. The comments are rendered one below the next.