Comments implementation review

toiletgranny commented 2 years ago

Intro

It felt so great today to finally see all the pieces we've been crafting in isolation, put together, dancing with each other, creating one delightful experience. It really does breathe a new life into the video page, doesn't it! I'm thrilled (and a bit frightened if I'm honest) to see our work filled with real words, and real conversations, between real people.

As with every implementation of this size, when all the building blocks are assembled together, it's only then you can notice new problems and challenges ahead. That's why I was so relieved when I realized that all the things I have found, are just minor and mostly stylistic issues. Of course, when combined as a whole, they make the entire experience a bit rough, but I think it's perfectly fine at this stage. That's why we're doing this. So, let's get started!

Video info section

On iOS, Safari, there’s something wrong with how text is rendered in the collapsed form of the Video info section. Have a look: 🏞 https://user-images.githubusercontent.com/35307309/171630772-d3375dbe-72ad-4ecf-9262-22131f692674.jpg
Also, still on iOS, Safari, when I click the "Show more" button, and then "Show less", the video description is gone entirely.

Video reactions

As is: There's a min-width: 155px set on the Reaction stepper component. To be: There should be no min-width, as it makes the nested buttons misaligned horizontally. I'm curious: why was this property set over there in the first place? I'm worried it might have been solving some other CSS issue I'm not aware of.
As is: When leaving a video reaction, all Reaction chip components in comments are disabled. To be: Let's not do that. Let the user choose a reaction, and if the blockchain transaction is still processing, let's just trigger the “Sign outstanding transactions” dialog.
🚨 As is: The Reaction stepper component doesn’t update after leaving a reaction and having the transaction processed successfully. When I refresh the page, the Reaction stepper component displays correct values. To be: The Reaction stepper should be refreshed the moment the transaction has finished processing.

Comments

General:
1. 🚨 As is: Clicking on the timestamp changes the URL instantly. As a consequence, the page layout and sorting changes instantly, and there's a lot of confusion as the page is moving up and down, side to side, like a roallercoster. To be: Clicking on the timestamp should copy the URL into the clipboard, at most. It can also just do nothing and be a hyperlink, so that — as a viewer — I can simply right-click and copy the URL from the browser's context menu.
2. As is: The date time tooltip which appears upon hovering over the timestamp, seems to be used in a wrong variant: multiline. This causes the tooltip to be taller and wider (because of the increased padding), that it should be. To be: The tooltip component, in this scenario, should be used in the non-multiline variant. To be checked: I think I saw the same issue somewhere else, which makes me think if this is not the Tooltip component implementation being flawed. Perhaps the Multiline variant is now the default one, in which case — it shouldn't be.
3. As is: The color of the dot separating the username and the timestamp is set to var(--color-background-alpha). To be: var(--color-background-strong-alpha)
4. As is: Having selected “Most popular” as a sorting method, the comments which are equally “popular” seem to be sorted with the newest ones on top. To be: It's best if equal comments are sorted by their date, with the oldest ones on top (just like the replies).
5. As is: On smaller viewports, the “Reply” button is carried over to the next line in the footer of the Comments body component. 🏞 https://user-images.githubusercontent.com/35307309/171634903-0a7f0c1a-ce75-4513-afc7-eafa8f3d8fe9.png To be: If there's enough space to fit both the reaction buttons and the replies-related elements together, they should be sitting in one line. Otherwise, a lot of vertical space is lost, which is especially harmful on mobile devices.
6. As is: Because of the tricky CSS and playing with a negative margin and additional padding (which, as a reminder, is added there to accommodate for the highlight around the comment), the comment row is no longer 100% width of the parent container. It’s slightly less than that. This is best observed when you add * { outline: solid 1px red; } to the inspect mode in Chrome. To be: Each Comment row component's width should fill the parent container.
7. Can we offset the Comment body component's footer horizontally by minus 8px? This is so that Reaction chip components appear aligned with the text above. I will update the designs accordingly, but please let me know if we can do that. I believe it's a tiny CSS tweak that makes a ton of difference.
Reactions:
1. As is: Reaction chip components are currently sorted using their original order in the Reaction popover component. Regardless of how many times each reaction has been applied under a given comment. To be: Reactions under a comment should be sorted from left to right, top to bottom, starting with the most popular reaction.
2. As is: Color of the icon in the new reaction button (the monochromatic smiley face) is derived from the Button component implementation: var(--color-core-neutral-50). To be: Can we override it with the one which is currently used in the designs for comments? It's var(--color-core-text).
3. As is: Color of the value in the Reaction chip component, in its default and inactive state, is set to var(--color-text-strong). To be: var(--color-text). The lighter color kicks in only upon hovering or when the Reaction chip component is active.
4. As is: The gap between the reactions part and the replies part in the footer of the Comments body component is set to4px. To be: It should be set to 12px. It is very important to have this gap between the reactions part and the replies part because if there are no replies under a comment, then the “Reply” button is displayed too closely to the reactions part of the footer. Please, have a closer look at the layers' structure for this component in Figma.
Replies:
1. 🚨 I can’t seem to reply to other replies. Only to the comments in the main thread. That’s not how it goes in user stories. Why is that?
2. 🚨 In the Replies chip component, there are no avatars of up to 3 members who left the highest number of replies under a given comment. Why is that?
3. As is: Color of the label in the Replies chip component is set to var(--color-core-neutral-300) in its default state. To be: var(--color-text)
4. As is: When my reply is published, my viewport is not scrolled to set the newly published comment in view. To be: It should be, accordingly to this user story.
5. As is: When entering a reply in the Comment input component, hitting the Cancel button doesn't trigger the Alert dialog component. To be: It should do that, accordingly to this user story.
Editing:
1. As is: The upper padding in the Edit history dialog body is set to 12px. To be: 24px, as it was set up on the Dialog component level. No overrides were applied here, at least not in the Figma designs. Is this an issue with a component, or with its usage?
Deleting:
1. As is: The List item component for deleting a comment is labeled as “Remove”. To be: “Delete”.
2. As is: A comment that is being deleted is not being grayed out for the time of processing the transaction. To be: It should be, accordingly to this user story..
3. As is: The delete confirmation Snackbar component doesn’t include a link to the video. To be: It should, accordingly to this user story.
4. As is: A deleted comment (with no replies) doesn’t disappear from view once the transaction has been finalized. Instead, it stays there but the contents change to “Comment deleted by the author”. To be: It should be gone momentarily, accordingly to this user story.
5. As is: In a deleted comment, which remains in the main thread, the gap between the “Comment deleted by the channel owner/comment author” message and the header part of the Comment body component is unchanged, set to 8px. To be: It should be set to 12px, just like the gap between the deletion message and the header part of the Comment body componet.
Snackbars:
1. 🚨 I couldn’t test error snackbars (which I guess is good news overall, right?), so: let’s just make sure all error snackbars have the copy and actions which have been specified in the designs. Designs for error snackbars can be found at the end of every user story mentioning a Polkadot transaction.
Notifications:
1. As is: When landing on the video page after being redirected there from the notification about a comment, or just a URL targeting a specific comment, the viewport is not being scrolled to put the comment in view. Also, the Comment row component is not highlighted. To be: Both things should happen, accordingly to this user story.

Overall

As is: At no point I was greeted by the "We save social interactions  on blockchain" popover. To be: I should see this popover if this is the first time I'm interacting with either reactions for the video, or reactions for comments, accordingly to this user story (video reactions) and this user story (comments reactions).
As is: The number of comments in the header for the comments section does not include replies. To be: It should.
🚨 It was only once I got a chance to actually write a few comments myself, and attempt to leave a few reactions under other comments when I realized: oh man, @kdembler, you were right! Signing transactions takes forever! I'm deeply concerned about how discouraging this will be for viewers to get involved in social interactions on Atlas. If there's nothing we can do on the technical side, as we've already discussed this a couple of times, I started thinking if there's anything on the surface experience to mitigate this. And that's when I thought of Google, Gmail, and how they're faking sending an email to let you undo it within ~7 seconds after you hit "Send". Guys, could we try doing the same thing here? Here's how I think this could work:
1. As a viewer, you hit the Reaction chip component to leave a reaction.
2. Immediately after that the Polkadot window appears so that you can sign the transaction. On our side, we're changing the state of the Reaction chip component to processing, and we throw a snackbar letting the viewer know that the ball is on their court.
3. So far, points 1. and 2. are the same as with the current implementation. But once the viewer signs the transaction in Polkadot, instead of keeping the Reaction chip component spinning, and the other ones locked, waiting for the transaction to be actually processed... how about we act as if it was it? Done? We show the reaction as applied, the other reaction chip components are unlocked and you, as a viewer, are good to go. Because, when you think about it, what else could we want from you, as a viewer, at this point? What are the chances that once you've signed the transaction it fails? And even if it does, then so what? We still have the error snackbar to let you know about it, right? And the result would be exactly the same as if we were to keep you waiting, right? In return, however, you get an experience that feels much more snappy and responsive. And, most importantly, not discouraging you from doing it again. I don't know... Is it feasible? @kdembler @dmtrjsg What am I missing with this?

kdembler commented 2 years ago

Thanks for such an in-depth review Adam, this was badly needed as you can see, because we broke some things along the way when bringing it all together. 🎖️ 🏅 🥇

Video info section

Both points captured in https://github.com/Joystream/atlas/issues/2780

Video reactions

Comments

General
Reactions
Replies
1. As discussed yesterday, we will drop this from the initial scope. Created an issue here: https://github.com/Joystream/atlas/issues/2798
2. As discussed yesterday, we are limited by the QN here and we will drop from initial scope as well. Issue here: https://github.com/Joystream/atlas/issues/2799
3. https://github.com/Joystream/atlas/issues/2796
4. We couldn't reproduce this
5. https://github.com/Joystream/atlas/issues/2800
Editing
1. https://github.com/Joystream/atlas/issues/2796
Deleting
1. All captured in https://github.com/Joystream/atlas/issues/2801
Snackbars
1. https://github.com/Joystream/atlas/issues/2802
Notifications
1. https://github.com/Joystream/atlas/issues/2803

Overall

toiletgranny commented 2 years ago

Perfect, thanks! Little things, but I appreciate the way you formatted the comment. Made it so much easier for me to see what's what.

Joystream / atlas