Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Create comment page refactor #1165

Merged
merged 10 commits into from
Apr 17, 2024
Merged

Create comment page refactor #1165

merged 10 commits into from
Apr 17, 2024

Conversation

hjiangsu
Copy link
Member

@hjiangsu hjiangsu commented Mar 4, 2024

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
simulator_screenshot_11035348-06C9-4B12-BE0B-BEEFBE4E4E40 simulator_screenshot_44F7F5FD-9615-4620-9B43-32BCE9C7407B
simulator_screenshot_04CEDB6F-B77B-47A1-9AF5-457395972889 simulator_screenshot_71311E3F-11F5-447E-94F5-F703E6837840
simulator_screenshot_82411B07-723F-4D80-8BC1-618C4382C51E simulator_screenshot_A097A0DD-4BA7-4068-AE4D-9B1E1E455953
simulator_screenshot_B5B4150B-7D1D-460C-BF91-E161712B17AA simulator_screenshot_7112944A-DF17-48AF-94B4-29F382A1F2FD

Checklist

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

@micahmo
Copy link
Member

micahmo commented Apr 11, 2024

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
Copy link
Member Author

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

@micahmo
Copy link
Member

micahmo commented Apr 11, 2024

Sounds good!

@hjiangsu hjiangsu force-pushed the refactor/create-comment-page-1 branch from 35e2e56 to a90f2ff Compare April 13, 2024 00:04
@hjiangsu hjiangsu changed the title Draft: Create comment page refactor Create comment page refactor Apr 16, 2024
@hjiangsu hjiangsu marked this pull request as ready for review April 16, 2024 17:20
@hjiangsu
Copy link
Member Author

@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
Copy link
Collaborator

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?)
    image

  • 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?
    image

  • 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
Copy link
Member

micahmo commented Apr 16, 2024

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.

image
  • 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
Copy link
Collaborator

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
image

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
Copy link
Collaborator

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

Copy link
Member

@micahmo micahmo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome work, I know this was a huge effort! I did review the code but haven't tested yet (I'm ok with getting this merged in to test).

I've disabled the comment action on the search page, and on the user feed page.

Does this also need to be done for inbox replies and mentions (the latter of which uses a totally different UI, not swipe gestures)?

lib/comment/utils/navigate_comment.dart Outdated Show resolved Hide resolved
lib/comment/view/create_comment_page.dart Show resolved Hide resolved
lib/comment/view/create_comment_page.dart Show resolved Hide resolved
lib/shared/snackbar.dart Outdated Show resolved Hide resolved
@hjiangsu
Copy link
Member Author

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
Copy link
Member Author

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
Copy link
Member Author

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

@hjiangsu
Copy link
Member Author

Before After
image image

@machinaeZER0 does this seem to be a bit better?

@hjiangsu hjiangsu merged commit 034ffe9 into develop Apr 17, 2024
1 check passed
@hjiangsu hjiangsu deleted the refactor/create-comment-page-1 branch April 17, 2024 15:15
@machinaeZER0
Copy link
Collaborator

Before After
image image

@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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants