-
Notifications
You must be signed in to change notification settings - Fork 56
Fixes #98: Redesigned and scrollable AppDetails page #131
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.
Code looks good, but I'm not sure whether hiding the toolbar by default is a good pattern. It looks good visually, but how do users know about using this pattern? Isn't it cumbersome to require an extra scroll action to make the scrollbar visible to press the close button?
@d4rken yeah I was thinking that too, should I change it back to expanded by default? While the design docs has an example where it’s okay to have both the toolbar and the title underneath, i think it looks a bit strange Also, I’ve always found the existence of close buttons itself strange as well, given that every android phone has a far more accessible back button to use instead. Considered removing the toolbar entirely and placing the options and stuff under the description Another option is to squeeze the app icon, name, etc into the toolbar but that’s problematic for longer names |
Up to you.
I agree, the expanded view on your 3rd screenshot looks weird.
I think it's complex. For starters the back button is not always located in the same position or rather same distance. Think tablets. Reaching it may be more cumbersome than accessing the toolbar. There are also different things happening in some cases depending on whether the toolbar "back" and the backbutton is used (lookup backstacknavigation).
Just thinking out loud:
Maybe we could also look at Google Play. I wonder if we could pull the header graphic from Google Play somehow... Some more graphics would give us more to work with design wise... |
@d4rken what do you think about 0a4735b? Updated the PR with new screenshots
Looked weird when I tried it because of how close to the FAB the buttons end up. I don't think many people actually attempt to contact developers, so perhaps the overflow menu is a better home for it, and the toolbar ends up looking neater.
When the title is long, it gets broken into two lines. Makes the toolbar a tad crowded but it works
I tried this, but with the icon inconsistencies and the fact that icons typically aren't designed to be displayed at the size needed to fill the header, it didn't look too great. The header with the category, tags and icon does collapse now though, which I demonstrate in one of the screenshots |
1st and 3rd looks good, middle not due to cutoff. What would look cool here would be fade out the icon and allow the header to collapse to a 1 toolbarheight. The FAB could detach and move to the bottom right of the screen... Or maybe the toolbar collapses to 1 tlb height and the icon + app name appear in the toolbar. Ellipsizing a too long title and limiting it to 1 line is okay in my opinion when we just have 1 tlb height of space. I think the title should have gravity START, not CENTER.
Okay.
Code looks good to me. |
That’s what happening in the middle screenshot, it’s using the
I centered it to match the centering of the other header elements
This could be cool, I’ll give it a try |
Oh that looks fancy! 👌 |
Will open a new issue (#138) for animating the app icon, for now I think this looks nice enough: Animation has the header elements fade: Thoughts? |
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 awesome 👌 👌 👌 |
When a lot of revisions happen from opening a PR to closing (ie the design changing significantly), is it best practice to keep the original post updated with the latest commit or post the changes in comments below? |
I'd say comments below, i.e. chronological, just like commits appear in the feed. That way someone following the PR can also understand the changes chronologically. Always editing the OP kinda means some information is lost, right? You made some nice screenshots, you could make a PR against |
Yeah that makes sense, will stick to that in the future Screenshots added in #139 |
Issues
#98: If description is long or while in landscape mode, the bottom part of AppDetails is cropped and inaccessible
Changes
New AppDetails page layout
Screenshots and details area now vertically scrollable
Contact button now in the overflow menu
Updated support libraries
Original PR (151e24b):
A few commits later (0a4735b):
Final (2f329eb):