Create comment page refactor

hjiangsu commented 2 months ago

Pull Request Description

This PR refactors the existing create comment page and attempts to solve the following issues:

It updates the create comment page to match logic similar to the create post page. This means that draft logic is all contained within the create comment page. It also means that all logic related to comment creation is contained within the page itself. This allows us to perform comment actions from anywhere without relying on passing in the correct blocs.
- When a comment is created or edited successfully, it calls a callback function onCommentSuccess. The parent widget can use this callback function to update the state of the parent (e.g., add comment to list, update comment content for edits)
Adds the ability to select a language for a comment. This brings us up to parity with the create post page.
Updated the UI/UX to better match the overall UI of Thunder.
- When replying to a post, the post information will be displayed in a similar fashion to the post page. It uses the pre-existing PostSubview widget, so any changes to the PostSubview will reflect on this page!
- When replying to a comment, the comment information will be displayed in a similar fashion to the comment cards. It uses the pre-existing CommentContent to display the comment, so any changes there will also reflect on this page!

Most of the logic is now in place, and should be more or less functional! Some standing issues to be fixed:

Need to add logic to update comment from the account page

Additional notes:

I've disabled the comment action on the search page, and on the user feed page. My main reasoning here is that I'd like for users to visit the comment within it's full context in order to perform actions on it (vote, reply, etc). This also simplifies some of the overall logic and reduces edge cases! Do let me know your thoughts on this change though.

Issue Being Fixed

Issue Number: #1012, #738

Screenshots / Recordings

Before	After

Checklist

[ ] Did you update CHANGELOG.md?
[ ] Did you use localized strings where applicable?
[ ] Did you add semanticLabels where applicable for accessibility?

micahmo commented 1 month ago

Just an FYI, I want to work on a "comment as other user" feature as some point. Let me know if you think I should wait for this PR or go ahead now.

hjiangsu commented 1 month ago

I think it might be good to hold off on that until this is complete! I'll probably get back on this soon

micahmo commented 1 month ago

Sounds good!

hjiangsu commented 1 month ago

@micahmo this should be more or less ready now (with the exception of the standing issues). I've updated the description to provide some more information/context. This is a relatively large change, so up to you if you want to do a code review on this!

@machinaeZER0 since I've updated the overall UI/UX, please let me know your thoughts on this!

machinaeZER0 commented 1 month ago

Hello! This is looking really nice :) Just a few thoughts/clarifications from me:

I definitely see the usefulness of a language setting given the issues its absence is causing - I do wonder if there's a classier way to integrate it, though. are we able to default the language in a given community/instance to something acceptable, or no? And what are your thoughts on making the language selector a dropdown icon next to your personal info, rather than a text dropdown between your info and the composition section? (mostly unsure of what the icon would be - maybe just a three dot guy?)
I think this is naturally improved having switched from an outline to a filled box style, but I think the quoted text isn't vertically centered. If that's true (might be my eyes playing tricks on me), is it possible to add a few pixels of padding to the bottom?
I'm sure this is outside the scope of this PR, but I still wish the send/post button was closer to the bottom, lolz. I think I'm in the minority on that one, but I like where it lives in most messaging apps from a reachability standpoint, rather than at the top of the screen. Maybe something to ponder for the future, but happy to keep things as they are on that front for the foreseeable :)

Let me know if I missed anything else you specifically wanted reviewed, but hope this was helpful!

micahmo commented 1 month ago

Hey all, I'm still reviewing this PR, but I wanted to respond to @machinaeZER0's comments real quick.

are we able to default the language in a given community/instance to something acceptable

Given that the Lemmy UI does not, we probably wouldn't either.

what are your thoughts on making the language selector a dropdown icon next to your personal info

As long as it's obviously a language selector, I'm totally up for UI suggestions. Just keep in mind that this currently matches the Create Post page, so we'd have to change both. Also keep in mind that this page will also eventually have the "comment as other user..." feature, so there will be a chevron to the right of the username (where you circled) indicating you can pick a different user.

I still wish the send/post button was closer to the bottom

I also agree with this! A while back I did back I tried a mockup where the send button integrated with the preview button. I'll have to dig that up. Again, that would be a common change between this and the Create Post page, so probably a different PR as you mentioned.

machinaeZER0 commented 1 month ago

That all makes sense to me - if we're matching the "normal" Lemmy experience, thoughts on matching the language and styling of the dropdown here? A single dropdown (maybe pilled) that says "Select Language," rather than the current "Language: Select Language," I mean

I think it being shorter and contained within a pill would be positive changes, but also wouldn't be opposed to just doing the latter and keeping the longer copy of the former :)

machinaeZER0 commented 1 month ago

Made issue #1309 as a place to discuss the send/post button idea, also :)

hjiangsu commented 1 month ago

Thanks for the feedback @machinaeZER0!

are we able to default the language in a given community/instance to something acceptable, or no?

As @micahmo mentioned, we're mostly following what Lemmy UI does. I do believe we can make slight improvements to the language selector in future PRs (such as limiting the language options based on what is enabled on the instance/community).

Selecting a default language would be difficult because instances/communities can have more than one language. However, if the instance/community only has one language enabled, I can see making that the default!

And what are your thoughts on making the language selector a dropdown icon next to your personal info, rather than a text dropdown between your info and the composition section?

As @micahmo mentioned, the only issue here is that we'll be implementing ways to choose a different user to post the comment as (in the future). This would likely follow similar UI patterns to how the create post page looks like (with the chevron icon)

is it possible to add a few pixels of padding to the bottom?

I can try to adjust that a bit more - it could be because of the spacing of the title text which causes an optical illusion

hjiangsu commented 1 month ago

Does this also need to be done for inbox replies and mentions

It doesn't apply to the inbox/mentions. The reasoning here is that if you get a reply or a mention, you might already have the general context of what the original discussion was about, unlike the user feed page or the search page. Additionally, sometimes you might want to just quickly reply to a reply/mention to acknowledge that you've seen it!

I'm not sure that I've tested all the edge cases everywhere, but I'm hoping that most of it gets caught during testing/nightlies.

hjiangsu commented 1 month ago

I'll go ahead and merge this in as is, and we'll address any outstanding issues with future PRs!

hjiangsu commented 1 month ago

Before	After

@machinaeZER0 does this seem to be a bit better?

machinaeZER0 commented 1 month ago

Before After

@machinaeZER0 does this seem to be a bit better?

If it's possible to split the difference I might do that, but if that's literally one pixel's worth of difference then the original spacing is probably fine to keep as-is - probably better to not overly pad, anyway :) thank you for mocking it up, either way!

thunder-app / thunder