RFC: Enhanced library navigation - Option 2: Tabbed library page#3274
RFC: Enhanced library navigation - Option 2: Tabbed library page#3274Lurux wants to merge 4 commits intoMetrolistGroup:mainfrom
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Comment Tip You can get early access to new features in CodeRabbit.Enable the |
| selectedTabIndex = filterType.ordinal, | ||
| modifier = Modifier | ||
| .graphicsLayer { | ||
| translationY = scrollBehavior.state.heightOffset + TopAppBarExpandedHeight.toPx() + statusBarHeight |
There was a problem hiding this comment.
I'm a bit iffy about what I did to properly position the library tabs here, however I didn't find a better way to do it. Current behavior is the tabs "stick" to the top of the screen when scrolling down, but this should be easy enough to modify.
There was a problem hiding this comment.
What about M3's native scroll-sticky?
There was a problem hiding this comment.
I'll take a look ! Problem I had here is the tab bar is outside the LazyColumn because I didn't want to duplicate it, but for some reason it doesn't take the status bar and app bar in consideration like the LazyColumn does when positioning.
Depending on the intended effect we might revert to passing it to each screen like we did with the filters tags, but that feels kinda crappy and would probably (?) break the animations
There was a problem hiding this comment.
I did try putting it in the stickyHeader inside the LazyColumn but I seem to remember it still didn't position correctly
There was a problem hiding this comment.
I did try putting it in the stickyHeader inside the LazyColumn but I seem to remember it still didn't position correctly
I will check this tomorrow maybe I can figure out a better way for this
There was a problem hiding this comment.
I checked that the current version doesn't have any visual glitches like those you mentioned ofc, however I reckon that solution might be cleaner (although I hate the idea of needlessly duplicating that, even if it's just some passing around)
There was a problem hiding this comment.
I'm not sure swipe between tabs and indicator animations would work properly if the tab bar is recomposed along with the actual contents every tile we switch tab ? I'm not an expert there, will take a look later on
There was a problem hiding this comment.
The lambda passing is not really duplication, the TabRow itself still lives in one place, we are just passing a reference down like the old filterContent pattern did.
On the animation concern, since swipe is not implemented yet, there is no recomposition issue to worry about right now. When swipe does get added, it will need a HorizontalPager anyway and the architecture will have to change regardless of which positioning approach we go with. So I would not let that block the cleaner solution for now.
There was a problem hiding this comment.
Ok, I see, thanks for the insight. Will tinker with that after my current work 😄
Lurux
changed the title
|
See also #3267 (comment) and the following conversation for styling discussion |
This reverts commit cfa0b05.
Lurux
changed the title
3 participants
This PR implements option 2 (Tabbed library page) of RFC #3267 (Enhanced library navigation).
This allows users to switch between categories with a single tap, and makes filters more intuitive, giving them a consistent behavior across all pages of the library.
As per user feedback, commit cfa0b05 adds icons to the library tabs, although it is my opinion that adding icons to these library tabs makes the interface feel more crowded. Given the current filter chips don't feature such icons, the merits of including them here should be discussed. Of course, the presence of these icons could be made configurable, but these are hard-coded as of this first draft.
I have not implemented swiping between tabs as of yet, but this could be made configurable when swiping individual items/songs is disabled.
I have not implemented a "home" / mix / overview library tab as of yet, but such a tab could be re-introduced if we find something meaningful to house there, such as featuring the speed dial from the homepage or a few "top" songs (most listened) from the past week.
Resolves #3267