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

A few AppBar fixes #682

Merged
merged 4 commits into from
Aug 25, 2023
Merged

A few AppBar fixes #682

merged 4 commits into from
Aug 25, 2023

Conversation

micahmo
Copy link
Member

@micahmo micahmo commented Aug 24, 2023

Pull Request Description

This PR fixes a number of issues related to the AppBar.

  1. Navigating to a community only showed the sort mode but no title.
Before After
image image
  1. After navigating to a community and opening the drawer, closing it would show the back arrow, and pressing back would go to a blank screen.

Before

2-before.mp4

After

2-after.mp4
  1. The community FAB sort option has been changed to show the generic sort icon, matching the AppBar and the post FAB.

  2. Finally, the post view has been updated to match the community view, with the community + post title and sort type in the AppBar.

Before After
image image

Issue Being Fixed

Issue Number: N/A

Screenshots / Recordings

Checklist

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

cc @tom-james-watson for review.

Copy link
Contributor

@tom-james-watson tom-james-watson left a comment

Choose a reason for hiding this comment

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

Looks great to me! I would maybe drop the community prefix on posts themselves, eg the "memes: " from your screenshot. I don't feel strongly about it though.

@micahmo
Copy link
Member Author

micahmo commented Aug 24, 2023

I would maybe drop the community prefix on posts themselves, eg the "memes: " from your screenshot.

I went back and forth on this, including taking a look at the other apps. Here's what they do.

  • Connect: Shows only community name
  • Lemmotif: Shows the word "Post"
  • Liftoff: Shows community: post title (like what I did)
  • Jerboa: Shows the word "Comments"
  • Beyond: Shows the word "Post"
  • Summit: Shows only the sort label
  • Eternity (Infinity): Shows only the sort label
  • Voyager: Shows the comment count
  • Sync: Shows only the community name

Obviously we don't have to follow what everyone else does, but no other app I tried shows only the post title. Just food for thought!

@machinaeZER0 Do you have a preference here?

@machinaeZER0
Copy link
Collaborator

This is a great PR, good catch on that weird Back behavior!

In terms of what gets displayed at the top of post view, I agree that "community name: post title" feels a bit off - in theory you already know what community the post is from when you click it, and even without that I feel like the post title is duplicative (also shown below this section) and will often get cut off because the text will be too long (and it deffo shouldn't break to more lines)

Personally I would lean towards matching Jerboa (and Boost for Reddit) by showing "comments" and the comment sort! It's always felt a liiiiittle odd having that above and not below the post content itself, but I think it still makes sense/matches the placement across the rest of the app :)

@micahmo
Copy link
Member Author

micahmo commented Aug 24, 2023

Thanks for the feedback, I changed it to "Comments".

image

@micahmo micahmo requested a review from ajsosa August 24, 2023 16:34
Copy link
Collaborator

@ajsosa ajsosa left a comment

Choose a reason for hiding this comment

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

I love the mouse panic in the before video ha ha. Looks good.

@ajsosa ajsosa merged commit 4951dd2 into thunder-app:develop Aug 25, 2023
1 check passed
@micahmo micahmo deleted the fix/appbar-stuff branch August 25, 2023 02:37
@micahmo
Copy link
Member Author

micahmo commented Aug 25, 2023

Can folks keep an eye out to see if there's a little stutter when opening the comments now? I'm feeling it on my device and it seems to coincide with when the heading is populated (although we're building so many other things at the same time, it surprises me that this change would cause such a big stutter).

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

4 participants