codemirror-blocks icon indicating copy to clipboard operation
codemirror-blocks copied to clipboard

Comment UX

Open schanzer opened this issue 6 years ago • 9 comments

  • [ ] We should render top-level comments if they are unattached.
  • [ ] We should badge nodes which have attached comments (like in Snap!)
  • [ ] Right now there is no way to attach a comment to a node. There should be a keystroke and possibly right-click that brings up a comment UI.
  • [x] Relatedly, adding comments at the top level introduces a bunch of problems. In Racket, for example, inserting a line comment and then some text separated by a newline results in the line comment disappearing (since it's a comment, and those aren't visible!) and the text on the new line being rendered as roots. We should sanitize comment input to always produce a single block comment.

schanzer avatar Sep 23 '19 13:09 schanzer

So I looked at the way Snap does this, and they just have comments always visible, with a button to collapse the comment bubble to a single line: Screenshot from 2021-11-22 08-45-02

That seems to work fine and we could do something similar. It also makes the UI for adding/editing/deleting a comment really obvious.

However, it breaks down for comments on nested nodes:

Screenshot from 2021-11-22 08-45-13

We could do all kinds of fancy layout magic like they do for graph visualizations, but that would be a lot of effort. Would it be safe to ignore the nested comment overlay problem until a future milestone?

pcardune avatar Nov 22 '21 16:11 pcardune

Can we really not use the existing visual layout for comments, and just allow them to be edited in-place as they are now?

I find Snap's comment layout to be a really mediocre design, and baking the assumption that comments show as top-level blocks into the system feels like something we will likely regret later.

schanzer avatar Nov 22 '21 18:11 schanzer

I should have been more specific. I was referring specifically to

We should badge nodes which have attached comments (like in Snap!)

The way nodes with attached comments get "badged" in Snap is that the comments are just always visible... there isn't really a "badge" AFAICT.

pcardune avatar Nov 22 '21 20:11 pcardune

I figured that's what you meant. :)

I'm just not a fan of their layout when it comes to comments, but I'm also a pragmatist. I'm not taking a hard line against Snap here -- I'm asking whether our existing layout is really that much work to make editable.

schanzer avatar Nov 22 '21 20:11 schanzer

It's no problem to make our layout editable. But I'm not sure what the difference is between our layout and Snap's layout other than that our comments are hidden until you focus on the node. Otherwise the layout is exactly the same.

The problem I'm trying to solve is what "badging" is supposed to look like.

pcardune avatar Nov 22 '21 20:11 pcardune

some other questions about comment behavior:

  • can comments be dragged / dropped from one node to another? If yes, what happens when you drag a comment onto a node that already has a comment: does it append or replace?
  • how does one navigate to/from a comment? Can you use the same keyboard shortcuts for navigating between nodes to access a comment node?

The more I think about it, the more it seems like comments should behave (and be implemented) exactly the same way every other node in the ast is implemented.

pcardune avatar Nov 22 '21 21:11 pcardune

The vast majority of comments are inextricably linked to code. Yes, sometimes there's a giant top-level comment that's more of a note than anything else, but I'd wager at least 95% of comments are about a specific line, expression, or block of code. So my thinking here is focused on those, and not the exception.

I can't imagine comments being dragged and dropped, without their associated nodes being dragged and dropped with them. And if we allowed for this functionality, I feel like we'd wind up getting a ton of blocks that have been moved without their comment coming along for the ride. Treating them just like any node in the tree could lead to bad outcomes.

The way I'm thinking about them, you don't really "navigate to a comment". Instead, you navigate to the expression associated with that comment.

This feels like something we should hash out in our call tomorrow afternoon/evening.

schanzer avatar Nov 22 '21 22:11 schanzer

My idea isn't to unlink comments from the code, it's just to treat them (and implement them) like a regular ast node, which can appear at the top-level or as a child of another node.

A node with an "attached" comment, is just a node with a comment node in its children. Dragging the node drags the comment and all the other children of that node along with it as per usual. A comment that's not "attached" to a node is just like any other node that doesn't have a parent, and would appear as a top-level node in the editor.

Navigating to a comment node would be the same as navigating to any other node. Drag a comment to the trashcan to delete it, or select it first then delete. Type Enter to edit the comment. If there is no comment to edit, then click on the placeholder to fill one in, or drag a comment from somewhere else to fill it in.

pcardune avatar Nov 23 '21 01:11 pcardune

Ah - so you'd maintain the relationship using a parent-child setup. That makes a TON of sense. Yeah, I'm good with that.

schanzer avatar Nov 23 '21 01:11 schanzer