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

Add access to saved comments #519

Merged
merged 6 commits into from
Aug 16, 2023
Merged

Conversation

CTalvio
Copy link
Collaborator

@CTalvio CTalvio commented Aug 2, 2023

Kazam_screencast_00000.webm

This PR makes the "Saved" option on the user profile page a toggle that switches over to a view that can list both posts and comments.

@CTalvio CTalvio changed the base branch from main to develop August 2, 2023 00:16
@micahmo
Copy link
Member

micahmo commented Aug 2, 2023

I love this functionality! I'd like to hear @machinaeZER0's thoughts on the UI. Initially, it's not obvious that Saved is "selected" and that I have to press it again to "unselect" it and show "my" posts/comments.

@machinaeZER0
Copy link
Collaborator

I'm realizing I don't actually know how to access saved comments in the current build. Weird!

I like the idea of finding some way to keep all of these things accessible within the profile window! I'm not crazy about the fact that selecting Saved shifts the whole button over (feels jarring), but I would also agree with Micah that the theme (which I think is just the default) doesn't make the buttons pop enough to make it clear that Saved has been toggled on its own.

I believe that we're looking into changing the user Inbox page to be themed more like the Profile page, but I actually think we could take a page out of its book to tackle this issue? One nice thing about the Inbox (besides the color of the toggled view really popping) is that the selected category (replies/mentions/messages) resizes and gets a check mark next to it. Maybe we could try something like that? So it would be like:

[Posts] / [Comments] [Saved] (Saved is separated - basically as it looks in the video)

and then toggling Saved would be like:

[Posts] / [Comments] [Saved]

I think if we do it this way (or in some similar fashion) it's a little less flashy, but I think making Saved an obvious toggle means we don't need to shift buttons around, which feels a little more user friendly (in my opinion). Some other options to make Saved an obvious toggle would also work, but figured I'd mention Inbox since I kinda like those buttons :)

@micahmo
Copy link
Member

micahmo commented Aug 2, 2023

I'm realizing I don't actually know how to access saved comments in the current build.

I don't think it's currently possible, which is what @CTalvio is addressing here.

I like your ideas for UI improvement. My only concern with the checkbox is that it seems like you're adding something, when in reality you're completely changing what you're looking at. It's not clear what's shown when Saved is unchecked. Could we have a very explicit distinction between "Mine" and "Saved"? Maybe two levels of tabs?

Mine Saved
Posts Comments

@machinaeZER0
Copy link
Collaborator

machinaeZER0 commented Aug 2, 2023

I don't mind that idea at all! We'd lose some vertical space but it feels very clear if done that way. Maybe the top ones (mine/saved) are smaller style buttons as well, done in some way to establish that they're the parent toggle? (Thinking that making one of the rows smaller in some fashion loses us less of that real estate)

@CTalvio
Copy link
Collaborator Author

CTalvio commented Aug 2, 2023

I was planning on getting rid of the buttons in inbox and replacing them with the same selector used everywhere else.

I do not like the inconsistency of there being different tab selectors, I do not like that they are not the same size between each other, I do not like that they get a redundant checkmark when selected and not just the highlight. I do not like that they get re-sized depending on which one is selected.

Additionally, the checkmark selectors are supposed to be for filters. The inbox should in my mind allow deselecting all three, to show all three content types in a single list. If I used thes in the profile, the unselected state would imply that the list now contains both saved and your own comments.

I'll think about ways to make the use of the saved button more clear. Though I'd like to claim "minimalism" here and challenge the idea that that is necessary to begin with.

I'm not making it two levels.

@machinaeZER0
Copy link
Collaborator

Lol

@hjiangsu
Copy link
Member

hjiangsu commented Aug 2, 2023

I also agree with the overall direction that this discussion is heading. It feels like we can improve the UX around how to display saved content to make it a bit more obvious. While the animation itself is cool when you select "Saved", it was a bit confusing for me to understand what was happening until I watched the video a couple of times

I personally would not mind having two levels (as what @micahmo) suggested, if we are able to make the user header (icon, name, etc) collapse as we scroll down. That would give us more vertical space to play with while giving a distinct "hierarchy" for own/saved content

@CTalvio
Copy link
Collaborator Author

CTalvio commented Aug 2, 2023

How about this, text buttons with direction arrows and changing text describing where they will take you, along with slightly slower animation.

Kazam_screencast_00000.webm

@micahmo
Copy link
Member

micahmo commented Aug 3, 2023

I think that's definitely an improvement! 🎉

@hjiangsu
Copy link
Member

hjiangsu commented Aug 3, 2023

I think that definitely helps a lot more!

@CTalvio
Copy link
Collaborator Author

CTalvio commented Aug 6, 2023

I'm personally happy with this, so I'm up to merge so users can let us know if this is good.

@hjiangsu
Copy link
Member

@CTalvio If you can fix the merge conflicts, I can merge this in!

# Conflicts:
#	lib/community/widgets/community_sidebar.dart
#	lib/user/widgets/user_sidebar.dart
@CTalvio
Copy link
Collaborator Author

CTalvio commented Aug 16, 2023

@CTalvio If you can fix the merge conflicts, I can merge this in!

Done

Copy link
Member

@hjiangsu hjiangsu left a comment

Choose a reason for hiding this comment

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

LGTM! Just remember to format the code to pass the checks!

@hjiangsu hjiangsu merged commit 68f7e3f into thunder-app:develop Aug 16, 2023
1 check passed
@CTalvio CTalvio deleted the saved_comments branch September 7, 2023 20:26
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.

4 participants