Issue/9349 update notifications tabs (Part 2) #9421
Conversation
theck13
added
[Type] Enhancement
Notifications
[Status] Needs Code Review
[Status] Not Ready for Review
[Status] Needs Design Review
theck13
changed the title
|
Looks good @theck13 ! I'm wondering, while swiping between the tabs there is a space between each fragment. Is this space added by the system or are we adding it? |
|
We are adding that. I set it as 16dp to match the standard margin for a screen. We can change it to a different value if we want. Without any margin between the pages, swiping looked a little odd since each notification item in the list does not have a uniform height. |
| mTabLayout.addTab(mTabLayout.newTab().setText(getString(R.string.notifications_tab_title_follows)), | ||
| TAB_POSITION_FOLLOW); | ||
| mTabLayout.addTab(mTabLayout.newTab().setText(getString(R.string.notifications_tab_title_likes)), | ||
| TAB_POSITION_LIKE); | ||
| mTabLayout.addOnTabSelectedListener(new OnTabSelectedListener() { |
There was a problem hiding this comment.
We might want to move the setting of OnTabSelectedListener after setupWithViewPager call, otherwise, the TAB_POSITION_ALL will be triggered every time configuration changes (eg. if you are on UNREAD tab, and rotate device the TAB_POSITION_ALL case will be called, and after that TAB_POSITION_UNREAD) .
There was a problem hiding this comment.
Nice catch! That might be a better way to track usage of the tabs. Interestingly, the original segmented control implementation tracks exactly the way you described. We may want to keep the tracking as is in order to compare usage differences between the old segmented controls and new tabs layout directly. What do you think?
There was a problem hiding this comment.
Yeah, If we want to be able to compare existing implementation with a new one we should probably keep the tracking as is. Maybe we can submit a fix in a month or so (however long it takes to get a reliable set of data for comparison).
There was a problem hiding this comment.
That sounds good to me.
I think it looks good! |
SylvesterWilmott
removed
the
[Status] Needs Design Review
Fix
Add swiping between tabs to the Notifications list view as described in #9349. This is the second part of the issue, which extracts the single list with multiple filters into multiple lists and adds the ability to swipe between the tabs added in a previous pull request. See the animation below for illustration.
Test
WordPress.com
WordPress.org
Review
Only one developer and one designer are required to review these changes, but anyone can perform the review.
Release
The
RELEASE-NOTES.txtdocument was updated in 96a8f2a with the following: