search

netcreateorg / netcreate-itest

Developing the 2.0 version of NetCreate
https://github.com/netcreateorg/netcreate-2018
Other
0 stars 0 forks source link

Comment Status #157

Closed benloh closed 2 months ago

benloh commented 3 months ago

Comment Status Feature

Show "Unread Replies to me" and "Unread" in comment bar

screenshot_1491

The number of "Unread replies to me" and "Unread" are shown in the navbar.

screenshot_1494

Hover over the icons to see the labels explaining what the icons are.

screenshot_1502

If all comments are read, the comment status icons are grayed out.

Show new message alerts

screenshot_1497

When a new comment is added...

All Unread Messages Panel

screenshot_1501

Other Notes

Original First pass

A first pass at comment status.

Caveats

jdanish commented 3 months ago

Cool. Seems to work!

I know that you have a list of to-dos, and there are some prior issues related to notification, so ... do you have a list?
or I can wait for the next round.

Some other key things in case:

  1. We'll want to distinguish between new comments (show on comment icon status) and new replies to me (popup is cool). Reminder that in meme there is a 3rd category of replies to my model - in Net.Create it is one shared model so less of an issue.
  2. Local v Network replies as you noted, though if 1 is covered I think that would make it far less important.
  3. Actionable will be good eventually. Though a complication is that it is funny to show a comment without the node or edge that is tied to the comment. One option is to pop it up though that can be disruptive. Another is to put a button on the common that pops it up. The third is to not pop it up directly but have UI indications of where it is so you go and have the choice (e.g., if I click on the comment icon in the nodes table, I should know what node it is so it's not an issue.)
  4. A list is of course a good idea in case there were multiple changes since the last time you looked. Or some way to see a list / the planned comment table / list. I think we don't need any disappeared popups if there is an icon indicating new replies that can be clicked to see the comment table. Or an icon on the node / edge table (I don't love how cluttered that might feel, but it is the most intuitive I think if we just indicate there is a comment on the table and then you can find it, but I'll ponder).
  5. I think we'll want the option to disable or modify the duration of the popups via the template since the meme team still worries it'll be distracting.
benloh commented 3 months ago

At this point I think I have more questions than answers. So this is a good start. I think I want to pull out a lot of these as separate design issues so that we can tackle them discretely.

benloh commented 2 months ago

Second Pass

@jdanish here's a second pass. Please give a whirl and see what you think. Clicking on the comment to open it is not working yet. (Updated the main message too)

Show "Unread Replies to me" and "Unread" in comment bar

screenshot_1491

The number of "Unread replies to me" and "Unread" are shown in the navbar.

screenshot_1494

Hover over the icons to see the labels explaining what the icons are.

Show new message alerts

screenshot_1497

When a new comment is added...

All Unread Messages Panel

screenshot_1501

Clicking on the comment bar will open up the full list of unread comments. Clicking it again will close the panel. (Or you can use the "Close" button).

Other Notes

jdanish commented 2 months ago

cool. I like this direction. I am not sure we want to just use color on the icons, but I'll leave that be for now.

Two minor things: 1) I think identifying nodes by number isn't all that helpful. It should be Node: "Algae" instead of Node: 2 I think? 2) The empty space to the right of the icon on mouse over feels weird? Again, minor.

Thinking aloud I could see some debate about whether we want the double-count on "replies to me" but I think this approach makes the most sense as-is, and to only count it once would be more confusing, so let's leave it for now.

I'll wait to run past the team when the links work but I like this direction.

benloh commented 2 months ago
  1. I was worried long Node names would make the comment summary items difficult to read. But we can try it.
  2. Yeah I was trying to get fancy with animating the hover. Seems I can either live with the extra space, or lose the animation of the slide out. I'll play with it some more to see if there's a better solution.
jdanish commented 2 months ago

Long node names make the whole graph look annoying, so I think we tend to avoid them, but ... I'd be OK with truncating the name after some number of characters to avoid the problem? Like "Long name he..." for "Long name here be found" (I was just reading a review of a pirate tv show.

jdanish commented 2 months ago

The mark all read works nicely. Two new finds:

1: If you update a comment, it does pop up saying there is something new (correct) but does not update the unread count (still says 0, not as expected).

2: If you type a long comment it'll scroll off screen. We should either line wrap or truncate in the previews.

Screenshot 2024-04-17 at 9 17 10 PM
jdanish commented 2 months ago

Also, I realize the comment links don't yet work, but presume the node / edge names will also be working? If not (or later) that's fine since the comment link does the heavy lifting.

benloh commented 2 months ago

1: If you update a comment, it does pop up saying there is something new (correct) but does not update the unread count (still says 0, not as expected)

Updating an existing comment does not mark it unread, nor update the counts. So you're saying a comment should be marked unread whenever the content is edited?

jdanish commented 2 months ago

Yes, and that new commit looks good. Thanks!

jdanish commented 2 months ago

Eventually, I think it likely we request that instead of "Edge 1" it read "Edge Algae -> Fish" however ... I'd say that can wait until after we lock down the rest of the comment stuff unless it feels much easier to do now. Just throwing that out there. I'd rather get to where we can test comments, start editing templates outside of code, and then port into meme otherwise. Thanks!!

jdanish commented 2 months ago
Screenshot 2024-04-18 at 9 35 09 AM

I think the node not found were because the comments were deleted by the other user before this one read them. I can dig in but figured I'd first share in case it helps. I can look closer later if the issue isn't obvious.

This is live now at http://198.211.109.198/graph/SeedsTest/#/edit/1-1-EVM if you want to check.

benloh commented 2 months ago

Third Pass

Integrating these into the main comment so we have all notes in one place.

jdanish commented 2 months ago

This looks awesome. Testing hard with team today. I believe the one minor tweak we are waiting on from late last night is having a view button on nodes table for parity? Otherwise we might be close to functionality completeness here?

jdanish commented 2 months ago

By the way - I am not 100% sure we still need / want a "comment list" function now that this is all working as it is, but will discuss with the meme team. It might be meme only ... will follow up but I think that's lower on your list anyhow so no rush?

benloh commented 2 months ago

I believe the one minor tweak we are waiting on from late last night is having a view button on nodes table for parity? Otherwise we might be close to functionality completeness here?

I just added the "View" button.

So yes, the functionality is basically complete, noting that:

jdanish commented 2 months ago

Sounds good! I've been testing on and off the last few days and it seems stable now, so I am OK with a merge if you are. If you prefer I hit the current build harder, I can also do that.

jdanish commented 2 months ago

@benloh Is there an easy way to verify the correct deletion of comments on deletion of node? I am not seeing any errors, but I also don't know if there are random comments hanging around in the DB?

NOTE: It occurred to me that one might expect comments to merge when merging (just like edges) but I think that's going a bit too far and think this works as-is.

benloh commented 2 months ago

@benloh Is there an easy way to verify the correct deletion of comments on deletion of node? I am not seeing any errors, but I also don't know if there are random comments hanging around in the DB?

Sorry I should have included testing procedures.

The best way to test this is test it BEFORE the fix:

  1. First check out the commit that doesn't have this feature: a0250160200563ddba60793cdfe16a768416a37e
  2. Add a new node and note the node number (e.g. # 10)
  3. Add a comment to that node, e.g. "This is a comment on # 10"
  4. Add another new node (e.g. # 11)
  5. Add a comment to that node, e.g. "This is a comment on # 11"
  6. Now delete those two nodes. (You might have to enter Admin mode)
  7. CTRL-C to quit the server
  8. Restart NetCreate via npm run dev
  9. Add a new node: There should be a new node # 10 because the system restarts the count based on the last node.
  10. Note the new node already as the "This is a comment on # 10" comment in the node.
  11. Add a second new node. Again, # 11 will be along with an existing comment.

Now do the same thing with the fix.

  1. Check out 9e1735903e1486afeb14e1b8cfd2d9aa94c723d9
  2. Do the same thing: Add new nodes, comments, delete the node, quit the server, restart the server, add nodes.
  3. The new nodes should NOT have comments leftover.
  4. Add a second new node.

NOTE: It occurred to me that one might expect comments to merge when merging (just like edges) but I think that's going a bit too far and think this works as-is.

Yeah simpler is probably better.

That also reminds me that I had tested the research log earlier, but it might be worth another check to make sure it's doing what you expected. So if comments are deleted you can still review the log to see what was there.

jdanish commented 2 months ago

Thanks! I can now verify that that appears to work and with a whole bunch of tinkering with deleting and creating nodes and restarting I don't see any new issues.

Will look at logs. Thanks!

benloh commented 2 months ago

The other way to review of course, is to just look at the loki file. But that's a little harder to suss.

Hopefully it's pretty solid now, but I'm sure there's some edge condition that we haven't yet caught.

jdanish commented 2 months ago

Yeah, that is a bit of a hassle for now. Maybe later when we have comment exports :)

And yeah, seems solid for now.