Sidebar: Minor Improvements #23611
Sidebar: Minor Improvements #23611
Conversation
| } | ||
| } | ||
| case .notifications: | ||
| // TODO: (wpsidebar) update tab bar item when new notifications arrive |
There was a problem hiding this comment.
We already do it in SidebarVC
| @@ -20,7 +20,6 @@ class SiteSplitViewContent: SiteMenuViewControllerDelegate, SplitViewDisplayable | |||
| func displayed(in splitVC: UISplitViewController) { | |||
| RecentSitesService().touch(blog: blog) | |||
|
|
|||
| // TODO: (wpsidebar) Refactor this (initial .secondary vc managed based on the VC presentation) | |||
There was a problem hiding this comment.
Added to a tech debt list.
| // -warning: List occasionally sets the selection to `nil` when switching items. | ||
| viewModel.$selection.compactMap { $0 } | ||
| .removeDuplicates { [weak self] in | ||
| guard $0 == $1 else { return false } |
There was a problem hiding this comment.
I think this is the easiest way to implement it without introducing any additional state.
There was a problem hiding this comment.
What do you think implementing it by handing double tap gesture?
At the moment, double tap would lead to selection getting the same value. But selection getting the same value doesn't always mean a double tap.
It's very possible that selection is changed to the same value, programmatically rather than by user interaction. For example, when tapping a reader post deep link while the app is already in the Reader, the code may assign .reader to the selection again.
There was a problem hiding this comment.
What do you think implementing it by handing double tap gesture?
It has to be single tap because that's what Apple apps do.
programmatically rather than by user interaction
That's a good point. I'm not sure how to address it because the way List selection in the sidebar works is also indirect. There is no "user tapped cell X" callback. It's observables all the way down.
There was a problem hiding this comment.
I think it's OK to ship it this way because if it's a deep link for let's say "Discover", it should be OK to pop to root. Both the deep links and Split View needs to be revised in the future versions.
There was a problem hiding this comment.
I only checked out Apple Podcasts app. And it has inconsistent UX. If you tap one of the tabs (i.e. Browse), the app takes you to the root. But if you tap other items (like in the Library section), it doesn't. I feel like Apple apps may piggy pack on UITabBarController's implementation.
I think it's OK to ship it this way because if it's a deep link for let's say "Discover"
Deep link is just one example on top of my head. From the code level, viewModel.selection = ... can be done at any stage, and it's very likely that we would forget this simple assignment may lead to a navigation stack change to the app UI.
I guess it's okay to ship it as the potential issue is limited on the Reader tab only.
|
| App Name |  WordPress Alpha |
|
| Configuration | Release-Alpha | |
| Build Number | pr23611-58407bf | |
| Version | 25.3 | |
| Bundle ID | org.wordpress.alpha | |
| Commit | 58407bf | |
| App Center Build | WPiOS - One-Offs #10701 |
|
| App Name |  Jetpack Alpha |
|
| Configuration | Release-Alpha | |
| Build Number | pr23611-58407bf | |
| Version | 25.3 | |
| Bundle ID | com.jetpack.alpha | |
| Commit | 58407bf | |
| App Center Build | jetpack-installable-builds #9745 |
| // -warning: List occasionally sets the selection to `nil` when switching items. | ||
| viewModel.$selection.compactMap { $0 } | ||
| .removeDuplicates { [weak self] in | ||
| guard $0 == $1 else { return false } |
There was a problem hiding this comment.
What do you think implementing it by handing double tap gesture?
At the moment, double tap would lead to selection getting the same value. But selection getting the same value doesn't always mean a double tap.
It's very possible that selection is changed to the same value, programmatically rather than by user interaction. For example, when tapping a reader post deep link while the app is already in the Reader, the code may assign .reader to the selection again.
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
1. Tint Color
Revert tint color to black in jpios and find a workaround for Reader sidebar in dark mode on iOS 17 (was showing white on white).
iOS 18
iOS 17 (Before – Broken)
iOS 17 (Now – Fixed)
2. Pop to Root
Pop current .secondary VC to root when you tap the same sidebar item.
pop-root-reader.mp4
3. Remove Me on wpios
It is now available in the main sidebar.
4. Fix disabled state for buttons
On login screens, the buttons were too dark. New state:
5. Remove focus effect in Site Menu
I'm adding a defect to fix it during the SwiftUI rework.
6. Store sidebar selection
The app will now remember when you open Reader persistently and re-open it authentically when you re-launch the app.
In the future, we probably need to simply persist all selections, but that would requires
TaggedObjectIdto beCodableor find another way to persist it.To test:
Regression Notes
Potential unintended areas of impact
What I did to test those areas of impact (or what existing automated tests I relied on)
What automated tests I added (or what prevented me from doing so)
PR submission checklist:
RELEASE-NOTES.txtif necessary.Testing checklist: