Ensure current site is always displayed in Sidebar List #24277
Conversation
- Update `BlogListViewModel.swift` to link `sidebarViewModel` and add logic for `currentSite`. - Modify `SidebarViewController.swift` to set `blogListViewModel.sidebarViewModel` during initialization. - Refactor `topSites` logic to ensure the current site is included, recent sites are prioritized, and all sites are sorted alphabetically.
…569-ensure-current-site-is-always-displayed-in-sidebar-list
| var encounteredIDs = Set(topSites.map(\.id)) | ||
| for site in allSites where !encounteredIDs.contains(site.id) { | ||
| if topSites.count >= SidebarView.displayedSiteLimit { | ||
| var displaySites = [BlogListSiteViewModel]() |
There was a problem hiding this comment.
Have you tried the steps from #23569? I vaguely remember this code, but I think we opened it just in case. I doubt it's a real issue. recentSites always has your most recently selected site as the first item of the array. It's impossible for it to not get displayed. I might be wrong – not entirely sure about this code.
There was a problem hiding this comment.
Yes, I followed the steps from the original issue, and I confirm I was able to replicate the issue, especially if you have multiple sites and you create or select a new one that starts with W or Z.
Screenshot:
|
| App Name |  WordPress Alpha |
|
| Configuration | Release-Alpha | |
| Build Number | pr24277-8aeec84 | |
| Version | 25.8 | |
| Bundle ID | org.wordpress.alpha | |
| Commit | 8aeec84 | |
| App Center Build | WPiOS - One-Offs #11793 |
|
| App Name |  Jetpack Alpha |
|
| Configuration | Release-Alpha | |
| Build Number | pr24277-8aeec84 | |
| Version | 25.8 | |
| Bundle ID | com.jetpack.alpha | |
| Commit | 8aeec84 | |
| App Center Build | jetpack-installable-builds #10818 |
| } | ||
|
|
||
| // Add recent sites up to the limit | ||
| for site in recentSites { |
There was a problem hiding this comment.
(nit) you should be able to merge these two loops in one: for site in recentSites + allSites
There was a problem hiding this comment.
Good suggestion f802776
| return nil | ||
| } | ||
|
|
||
| return allSites.first { $0.id == selectedBlogID } |
There was a problem hiding this comment.
(nit) I'd probably suggest using try mainContext.existingObject(with: objectID) instead of iterating over allSites.
There was a problem hiding this comment.
👍 implemented here 5bcfca3
| @@ -70,4 +70,120 @@ final class BlogListViewModelTests: CoreDataTestCase { | |||
| let displayedNames = viewModel.allSites.map(\.title) | |||
| XCTAssertEqual(displayedNames, [".Org", "51 Zone", "A", "C"]) | |||
| } | |||
|
|
|||
| func test_topSites_includes_current_site() throws { | |||
There was a problem hiding this comment.
(nit) please use camel case. Btw, for new tests (new suites), feel free to use Swift Testing.
There was a problem hiding this comment.
Updated here 8aeec84 👍
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
kean
deleted the
issue/23569-ensure-current-site-is-always-displayed-in-sidebar-list
branch
Fixes #23569
This PR modifies the sidebar site display logic to ensure the currently selected site is always visible, regardless of its position in the recent sites list or alphabetical order.
Changes Made
Testing Instructions
On iPad/simulator
SidebarView.displayedSiteLimitto a number smaller thanRecentSitesService.maxSiteCountScreenshot
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: