-
Notifications
You must be signed in to change notification settings - Fork 1.2k
[Site Design Revamp] Remove preview mode selector from nav bar #18455
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
[Site Design Revamp] Remove preview mode selector from nav bar #18455
Conversation
…ode picker for previews.
You can test the changes in Jetpack from this Pull Request by:
|
23f83ba to
af51a87
Compare
You can test the changes in WordPress from this Pull Request by:
|
mkevins
left a comment
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.
The code changes look good, and this works as expected (tested on iPhone 11, and iPad Pro (12.9-inch) simulator). I had a question about the added test, but it isn't a blocker.
One subtle thing I noticed, that could be different from the Android implementation, is that the selected preview mode in the preview screen gets reset every time it is re-opened (on Android, this is "remembered" when back is pressed, since the underlying state is persisted in the thumbnail screen). I think either behavior is fine, fwiw, and I'm just noting this for completeness.
Thanks @mkevins - that's a good observation and a pattern I hadn't considered. I imagine users will want to stick to the preview mode as they try out different designs, so I'm going to update this PR to match that functionality. |
|
Just a remark that I can review the additional updates when they are pushed 🙇 |
Thank you @ovitrif! This is ready for review again. The changes include:
|
ovitrif
left a comment
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.
Great work @twstokes ! 🎉
Works as expected in tests. Let's ![]()
...s/ViewRelated/Site Creation/Design Selection/SiteDesignContentCollectionViewController.swift
Show resolved
Hide resolved
|
A few CI checks failed, seems nothing caused by the changes, thus I retriggered them 🤞 |
…-screen-ui [Site Design Revamp] Update title text and remove subtext
Thanks @ovitrif for the review and for retriggering the tests! Looks like they're all good now. ✅ |
This PR:
To test:
Mobile thumbnails - Phone
Mobile thumbnails - iPad
Tracks events - Phone
enhanced_site_creation_site_design_viewedto be fired withpreview_modeset tomobileenhanced_site_creation_site_design_preview_viewedto be fired withpreview_modeset tomobileandtemplateset to the corresponding designenhanced_site_creation_site_design_preview_loadingto be fired withpreview_modeset tomobileandtemplateset to the corresponding designenhanced_site_creation_site_design_preview_loadedto be fired withpreview_modeset tomobileandtemplateset to the corresponding designenhanced_site_creation_site_design_preview_mode_button_tappedto be fired withpreview_modeset tomobileenhanced_site_creation_site_design_preview_mode_changedto be fired withpreview_modeset totabletenhanced_site_creation_site_design_preview_loadingto be fired withpreview_modeset totabletandtemplateset to the corresponding designenhanced_site_creation_site_design_preview_loadedto be fired withpreview_modeset totabletandtemplateset to the corresponding designenhanced_site_creation_site_design_selectedto be fired withtemplateset to the corresponding designTracks events - iPad
enhanced_site_creation_site_design_viewedto be fired withpreview_modeset tomobileenhanced_site_creation_site_design_preview_viewedto be fired withpreview_modeset tomobileandtemplateset to the corresponding designenhanced_site_creation_site_design_preview_loadingto be fired withpreview_modeset tomobileandtemplateset to the corresponding designenhanced_site_creation_site_design_preview_loadedto be fired withpreview_modeset tomobileandtemplateset to the corresponding designenhanced_site_creation_site_design_preview_mode_button_tappedto be fired withpreview_modeset totabletenhanced_site_creation_site_design_preview_mode_changedto be fired withpreview_modeset tomobileenhanced_site_creation_site_design_preview_loadingto be fired withpreview_modeset tomobileandtemplateset to the corresponding designenhanced_site_creation_site_design_preview_loadedto be fired withpreview_modeset tomobileandtemplateset to the corresponding designenhanced_site_creation_site_design_selectedto be fired withtemplateset to the corresponding designRegression 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)
testSiteDesignPreviewDeviceIsAlwaysMobilePR submission checklist:
RELEASE-NOTES.txtif necessary.