-
Notifications
You must be signed in to change notification settings - Fork 68
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
A few AppBar fixes #682
Conversation
There was a problem hiding this 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.
I went back and forth on this, including taking a look at the other apps. Here's what they do.
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? |
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 :) |
There was a problem hiding this 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.
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). |
Pull Request Description
This PR fixes a number of issues related to the AppBar.
Before
2-before.mp4
After
2-after.mp4
The community FAB sort option has been changed to show the generic sort icon, matching the AppBar and the post FAB.
Finally, the post view has been updated to match the community view, with the community + post title and sort type in the AppBar.
Issue Being Fixed
Issue Number: N/A
Screenshots / Recordings
Checklist
semanticLabel
s where applicable for accessibility?cc @tom-james-watson for review.