My Site Dashboard: Refactor dashboard's data source - #18310
Conversation
Instead of `DashboardItem` which is harder to deal with
This reverts commit 8bc09ba.
You can test the changes in Jetpack from this Pull Request by:
.ipa file can also be downloaded directly here.If you need access to App Center, please ask a maintainer to add you. |
You can test the changes in WordPress from this Pull Request by:
.ipa file can also be downloaded directly here.If you need access to App Center, please ask a maintainer to add you. |
| enum DashboardCard: String, CaseIterable { | ||
| case quickStart | ||
| case prompts | ||
| case todaysStats = "todays_stats" |
There was a problem hiding this comment.
Do we need the "todays_stats" here?
There was a problem hiding this comment.
@leandroalonso This raw value is currently only used for analytics events. And since I'm not sure when this will be released, I didn't want to have any differences between our events before and after this is released. That's why I left this.
What do you think though? Do you think it's unnecessary?
There was a problem hiding this comment.
Oh got it, let's def not change analytics values. If we remove it we need to refactor the track to keep the same value as before. If it's easier to just leave that here, let's just leave it.
| } | ||
|
|
||
| func createSnapshot(from cards: [DashboardCardModel]) -> DashboardSnapshot { | ||
| let items = cards.map {DashboardItem.cards($0)} |
There was a problem hiding this comment.
[nit]
| let items = cards.map {DashboardItem.cards($0)} | |
| let items = cards.map { DashboardItem.cards($0) } |
| let items = cards.map {DashboardItem.cards($0)} | |
| let items = cards.map {DashboardItem.cards($0)} |
| }) | ||
| let ghostCellsCount = ghostCells?.count ?? 0 | ||
| return ghostCellsCount > 0 | ||
| return currentCards.contains(where: {$0.cardType == .ghost}) |
There was a problem hiding this comment.
Again, you can delete that. Sorry for leaving that and causing you some trouble! 😅
|
Apparently, it wasn't just our posts cards that are duplicated, Github got inspired by it. |
That's some next level AI 😄 |
momo-ozawa
left a comment
momo-ozawa
left a comment
There was a problem hiding this comment.
Thank you for refactoring! Looking good!
|
|
||
| let section = NSCollectionLayoutSection(group: group) | ||
| let isQuickActionSection = viewModel.dashboardItem(for: sectionIndex) == .quickActions | ||
| let isQuickActionSection = viewModel.isQuickActionsSection(sectionIndex) |
4 participants
Part of #18308
Description
shouldShowlogic for all cards. Borrows posts cards logic fromDashboardPostsCardCellBlogDashboardRemoteEntityto be hashable and used it to hashDashboardCardModelinstead of the dictionary.Testing Instructions
This PR introduces weird behavior in the dashboard. Because of the refactor, the same cards are shown multiple times. However, the first ones are empty due to the VC dequeue logic implemented. This will be fixed in a future PR.
Again, I would recommend holding off testing till the whole refactor is complete. By then we will have to test that everything behaves the same way it used to do.
Regression Notes
Potential unintended areas of impact
N/A
What I did to test those areas of impact (or what existing automated tests I relied on)
N/A
What automated tests I added (or what prevented me from doing so)
Will add unit tests in another PR.
PR submission checklist:
RELEASE-NOTES.txtif necessary.