Collapse browser toolbar on scrolling the card list#18037
Collapse browser toolbar on scrolling the card list#18037oakkitten wants to merge 1 commit intoankidroid:mainfrom
Conversation
Here I opted for an extra `LinearLayout` inside of the `AppBarLayout`
for a minimal and safe-contained change. Other options are available,
see the comment in the code.
I also moved the linear progress indicator to the bottom of the
`CoordinatorLayout` for the following reasons:
* As a child of `CoordinatorLayout`, which is a “super-powered
`FrameLayout`”, it doesn't shift other elements when it becomes
visible and disappears;
* It stays on the bottom and doesn't move with the scrolling cards;
* Before, visually it was positioned just inside of the grid of the
table, which looked a bit weird, and it has rounded corners and
thus appeared shorter than the table;
* It may be harder to move it along with the `RecyclerView`.
lukstbit
left a comment
lukstbit
left a comment
There was a problem hiding this comment.
LGTM, thanks!
Requesting @david-allison opinion as you worked on this extensively.
lukstbit
added
Needs Second Approval
| here it is not necessary to have another LinearLayout as a child. | ||
| Instead we could be setting app:layout_scrollFlags on the individual children. | ||
| However, the <include> tag can only override a very limited set of arguments, | ||
| so the alternatives here would be not using the <include> tag, |
There was a problem hiding this comment.
my two cents: the alternative of not using <include> is super reasonable. Most of the new screens don't use it, if that matters.
There was a problem hiding this comment.
A quick search says there are 15 files saying <include layout="@layout/toolbar" in the XML
There was a problem hiding this comment.
Most of the new screens don't use it
A quick search says there are 15 files saying <include layout="@layout/toolbar" in the XML
Yes, it is still used in some places, but most of the "new" ones use MaterialToolbar instead of including the toolbar.xml layout, which has some extra sutff that normally is not used.
There was a problem hiding this comment.
Ah I see. I just went this way for the self-contained minimal change, otherwise I personally have no opinion on which way is the best one. 🤷♀️
|
@oakkitten appears this PR got hung up on some very specific but not completely prescriptive feedback. I think it's a cool feature and I'd like to get it in but I know you are particular about your changes (and your quality standard is super high, so at least IMHO, that's totally justified). So I don't want to just open up this PR and slam some commits in to get it done as that might chafe ;-) So to be prescriptive: it seems like the feedback to not use include would address both hanging comments at once - there wouldn't be the need for the big comment explanation and it wouldn't use include. Do you think you could refactor this to use MaterialToolbar (I think that's right...), not use include, and to instead set scroll flags (I think that's correct?). I do like the idea of using what most of the other new screens are using (MaterialToolbar, I believe) so I think that's positive motion. I don't have implementation experience to decide details past that though, so whatever seems like it will work past that would work for me... |
mikehardy
added
Needs Author Reply
|
Hello 👋, this PR has had no activity for more than 2 weeks and needs a reply from the author. If you think this is a mistake please comment and ping a maintainer to get this merged ASAP! Thanks for contributing! You have 7 days until this gets closed automatically |
BrayanDSO
added
Needs a new dev
|
hey can i work on this |
|
I'll FINALLY have get this over the finish line in the next few days for #18709 Code is written, just waiting on a few pre-PRs to be merged |
6 participants
Purpose / Description
Collapse browser toolbar on scrolling the card list. The toolbar takes space and may take even more space when browser chips are introduced (see #17355, #12554).
Screen_recording_20250301_155316.mp4
Approach
Pretty straightforward, use
AppBarLayout. I used my old commit which had some issues, namely:I suspected that these issues would be resolved with moving to
RecyclerView, and it seems that they have.See also the commit message.
How Has This Been Tested?
Briefly tested on my device API 34 and emulator API 24. Requires more on-hand testing to ensure the above issues are truly gone.
Checklist