Skip to content
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 Name] Adjusts design picker buttons for treatment variation #18397

Merged
merged 10 commits into from
Apr 21, 2022

Conversation

antonis
Copy link

@antonis antonis commented Apr 19, 2022

Description

This PR adjusts the design picker Choose buttons to communicate to the user that their action will create the site.

Design Chooser(Control) Design Chooser(Treatment)
IMG_0188 IMG_EB02383F75C5-1
Design Preview(Control) Design Preview(Treatment)
IMG_0189 IMG_0187

To test

Treatment variation

  1. Toggle ON the Site Name feature
  2. Start the Site Creation flow
  3. Choose an intent for your site or skip
  4. Enter a site name or skip
  5. Select a design
  6. Verify that the choose button is changed to create site
  7. Preview a design and verify that the choose button is changed to create site

Control variation

  1. Toggle OFF the Site Name feature
  2. On steps 6 and 7 above verify that the buttons are not changed

Regression Notes

  1. Potential unintended areas of impact
    N/A

  2. What I did to test those areas of impact (or what existing automated tests I relied on)
    N/A

  3. What automated tests I added (or what prevented me from doing so)
    N/A

PR submission checklist:

  • I have completed the Regression Notes.
  • I have considered adding unit tests for my changes.
  • I have considered adding accessibility improvements for my changes.
  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

@antonis antonis added this to the 19.7 ❄️ milestone Apr 19, 2022
@antonis antonis requested review from kyleaparker and a team April 19, 2022 09:45
@wpmobilebot
Copy link
Contributor

wpmobilebot commented Apr 19, 2022

You can test the changes in Jetpack from this Pull Request by:
  • Clicking here or scanning the QR code below
  • Then installing the build number pr18397-fc7ddd3 from App Center on your iPhone

The .ipa file can also be downloaded directly here.
If you need access to App Center, please ask a maintainer to add you.

@wpmobilebot
Copy link
Contributor

wpmobilebot commented Apr 19, 2022

You can test the changes in WordPress from this Pull Request by:
  • Clicking here or scanning the QR code below
  • Then installing the build number pr18397-fc7ddd3 from App Center on your iPhone

The .ipa file can also be downloaded directly here.
If you need access to App Center, please ask a maintainer to add you.

@antonis
Copy link
Author

antonis commented Apr 19, 2022

Note: The PR will probably need to target release/19.7

@antonis antonis force-pushed the feature/site-name-design-buttons branch from a1a9b91 to 7a4701d Compare April 19, 2022 13:57
@antonis antonis changed the base branch from trunk to release/19.7 April 19, 2022 13:57
@antonis
Copy link
Author

antonis commented Apr 19, 2022

The skip button change was reverted with 7a4701d

@antonis antonis marked this pull request as ready for review April 19, 2022 15:34
Copy link
Contributor

@Gio2018 Gio2018 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for tackling this @antonis ! the change works as expected; just left a couple of comments for you to consider.

@@ -100,10 +100,13 @@ class TemplatePreviewViewController: UIViewController, NoResultsViewHost, UIPopo
}

private func styleButtons() {
let isLastSiteCreationStep = ABTest.siteNameV1.variation == .treatment(nil) && FeatureFlag.siteName.enabled
let mainButtonTitle = isLastSiteCreationStep ? NSLocalizedString("Create site", comment: "Title for the button to progress with creating the site with the selected design")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since we have a couple of hardcoded strings, including this one, would it be possible to group them in a private enum?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great suggestion @Gio2018 👍
Handled with babc542

@@ -100,10 +100,13 @@ class TemplatePreviewViewController: UIViewController, NoResultsViewHost, UIPopo
}

private func styleButtons() {
let isLastSiteCreationStep = ABTest.siteNameV1.variation == .treatment(nil) && FeatureFlag.siteName.enabled
let mainButtonTitle = isLastSiteCreationStep ? NSLocalizedString("Create site", comment: "Title for the button to progress with creating the site with the selected design")
: NSLocalizedString("Choose", comment: "Title for the button to progress with the selected site homepage design")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: the comment should have a period at the end.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Handled with babc542 👍


super.init(
analyticsLocation: "site_creation",
mainTitle: NSLocalizedString("Choose a design", comment: "Title for the screen to pick a design and homepage for a site."),
prompt: NSLocalizedString("Pick your favorite homepage layout. You can edit and customize it later.", comment: "Prompt for the screen to pick a design and homepage for a site."),
primaryActionTitle: NSLocalizedString("Choose", comment: "Title for the button to progress with the selected site homepage design"),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

similar comments here: the comment should have a period, and since this file has quite a few hardcoded strings, wondering if we can group them?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As above handled with babc542. Thank you for the feedback 🙇

@twstokes
Copy link
Contributor

👋 If we're going to land on a single string after the A/B tests I'd suggest not using this PR to keep things simpler, but I've created one with tests in case we might need it.

Copy link
Contributor

@twstokes twstokes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My only suggestion would be to consider making changes in SiteDesignPreviewViewController rather than TemplatePreviewViewController since it's a more specific subclass. LayoutPreviewViewController also relies on TemplatePreviewViewController but because of ordering it isn't affected. 👍

antonis added 2 commits April 20, 2022 14:54
…ign-buttons-tweaks

[Site Creation] Pass a boolean to determine Site Design primary button titles
@antonis
Copy link
Author

antonis commented Apr 20, 2022

👋 If we're going to land on a single string after the A/B tests I'd suggest not using this PR to keep things simpler, but I've created one with tests in case we might need it.

Thank you for your suggestions @twstokes 🙇
I agree that your approach is a lot cleaner (can be unit tested 🎉 ) and might also be useful in our future enhancements (even when we remove the experiment). I've merged your draft PR onto this one :)

@antonis antonis requested review from Gio2018 and twstokes April 20, 2022 12:48
@peril-wordpress-mobile
Copy link

Warnings
⚠️ This PR is assigned to a milestone which is closing in less than 4 days Please, make sure to get it merged by then or assign it to a later expiring milestone

Generated by 🚫 dangerJS

@antonis
Copy link
Author

antonis commented Apr 20, 2022

⚠️ Please check the response here (internal ref: p5T066-37N-p2#comment-12065) before merging to the frozen 19.7 release 🙇

Copy link
Contributor

@ovitrif ovitrif left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All good during testing (both with Site Name on / off) and the way the "shape" of the code improved is quite nice! 🚀

Thank you @antonis & @twstokes for your collaborative efforts on this, and @Gio2018 for the valuable feedback 🙇.

I'm approving the PR, but as per the PR merge policy for our iOS repos, the author would normally merge it.

Apparently we're still in need of a follow-up to this internal comment: p5T066-37N-p2#comment-12066

Copy link
Contributor

@twstokes twstokes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! 🙇 If I understand correctly about how the translation keys work, the interface should translate "Create Site" because that key has been translated before and the comment isn't part of the key.

primaryActionButton.setTitle(createsSite ? TextContent.createSiteButton : TextContent.chooseButton, for: .normal)
}

private enum TextContent {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, good suggestion @Gio2018 and implementation @antonis. Makes things a lot cleaner!

Copy link
Contributor

@Gio2018 Gio2018 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome @antonis ! :shipit: !

@mokagio mokagio merged commit a7c675f into release/19.7 Apr 21, 2022
@mokagio mokagio deleted the feature/site-name-design-buttons branch April 21, 2022 01:56
@mokagio
Copy link
Contributor

mokagio commented Apr 21, 2022

@antonis and @twstokes (writing a single comment in the main PR) this has been bundled as part of 19.7 beta 1 (19.7.0.1).

Thanks for your work 🙌

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants