-
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: Add media upload tour #18471
Conversation
Without steps
You can test the changes in Jetpack from this Pull Request by:
|
You can test the changes in WordPress from this Pull Request by:
|
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!
- Looks like you need to fix
QuickStartFactoryTests
- Once you release the new
WPMediaPicker
you can point to the released version before merging this PR
@@ -74,6 +74,8 @@ typedef NS_ENUM(NSInteger, QuickStartTourElement) { | |||
QuickStartTourElementNotifications = 23, | |||
QuickStartTourElementSetupQuickStart = 24, | |||
QuickStartTourElementRemoveQuickStart = 25, | |||
QuickStartTourElementMediaScreen = 26, | |||
QuickStartTourElementMediaAdd = 27, |
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.
MediaUpload might be a better name, wdyt?
QuickStartTourElementMediaAdd = 27, | |
QuickStartTourElementMediaUpload = 27, |
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.
Done! 724a60d
let step1DescriptionTarget = NSLocalizedString("Media", comment: "The menu item to select during a guided tour.") | ||
let step1: WayPoint = (element: .mediaScreen, description: step1DescriptionBase.highlighting(phrase: step1DescriptionTarget, icon: .gridicon(.image))) | ||
|
||
let step2DescriptionBase = NSLocalizedString("Select %@to upload media. You can add it to your posts / pages from any device.", comment: "A step in a guided tour for quick start. %@ will be a plus icon.") |
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.
[Q] Is it intentional that there no space between the "%@" and the "to"?
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.
It is yes.
String.highlighting
adds a space between the title and the icon passed. Given that the title is empty in this case, this means String.highlighting
will add an extra space either way.
Removing the space from the base sting was to mitigate this issue. This was simpler than modifying String.highlighting
to handle this specific case.
if blog.userCanUploadMedia { | ||
addButton.spotlightOffset = Constants.addButtonSpotlightOffset | ||
let config = UIImage.SymbolConfiguration(textStyle: .body, scale: .large) | ||
let image = UIImage(systemName: "plus", withConfiguration: config) ?? .gridicon(.plus) |
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.
[Q] Any particular reason we're using the system image over the gridicon?
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.
This reverts commit 0b812f0.
# Conflicts: # WordPress/Classes/ViewRelated/Blog/Blog Details/BlogDetailsViewController.h # WordPress/Classes/ViewRelated/Blog/QuickStartTours.swift
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.
Looking great! Can you also update the release notes? Otherwise, LGTM!
Part of #18388
Description
MediaPicker-iOS
that fixes Empty view incorrect placement MediaPicker-iOS#386Testing Instructions
Initial screen: Site Menu (With media)
Initial screen: Home (With media)
No Media
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.