-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Quick Start for Existing Users: Refactor Quick Start UI data source #18395
Conversation
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works as described! Left a couple of comments below.
...ress/Classes/ViewRelated/Blog/Blog Dashboard/Cards/Quick Start/QuickStartTourStateView.swift
Outdated
Show resolved
Hide resolved
case existingUser | ||
} | ||
|
||
struct QuickStartToursCollection { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if it would make more sense to define QuickStartToursCollection
as a protocol. Wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@momo-ozawa If we do this we'll have to move some logic out of QuickStartToursCollection
. Which is the logic of returning the collections based on the blog.
To do that, we would either:
- Move it to `QuickStartTourGuide. However, this class already has so many responsibilities!
- Create a new component, maybe
QuickStartToursFactory
or something like that.
Given this, let me know what you think. Personally, I'm fine with leaving QuickStartToursCollection
as a struct or making it a protocol and adding a new component to handle the relevant logic. And I can't see a clear winner between the two approaches.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hassaanelgarem Thanks for exploring some alternatives!
My preference would be for option 2 -- returning an array of QuickStartToursCollection
inside the QuickStartToursCollection
struct itself was tripping me up a bit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@momo-ozawa Done in 74e3d54 👍
WordPress/Classes/ViewRelated/Blog/Blog Details/BlogDetailsViewController+FancyAlerts.swift
Outdated
Show resolved
Hide resolved
# Conflicts: # WordPress/Classes/ViewRelated/Blog/Blog Dashboard/Cards/Quick Start/QuickStartTourStateView.swift # WordPress/Classes/ViewRelated/Blog/QuickStartChecklistViewController.swift # WordPress/Classes/ViewRelated/Blog/QuickStartTourGuide.swift
And add `QuickStartFactory` to be responsible for returning collections
# Conflicts: # WordPress/WordPress.xcodeproj/project.pbxproj
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! What do you think about adding an internal release note for the quick start refactor?
Good idea! Added in e8cb133 |
Part of #18388
Description
This PR doesn't introduce any visible changes. It refactors some of the logic behind how we display quick start tasks in both the quick start card and the expanded quick start view.
Testing Instructions
The following describes a general smoke test over Quick Start UI:
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)
N/A
PR submission checklist:
RELEASE-NOTES.txt
if necessary.