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

add: update onboarding task list copies and illustrations #44854

Merged
merged 8 commits into from Mar 14, 2024

Conversation

rjchow
Copy link
Contributor

@rjchow rjchow commented Feb 21, 2024

Submission Review Guidelines:

Changes proposed in this Pull Request:

Updated copies and illustrations according to latest designs: Y5pUYSJPsGEud1vknUZhi8-fi-3352%3A22233

How to test the changes in this Pull Request:

Using the WooCommerce Testing Instructions Guide, include your detailed testing instructions:

  1. Ensure that customise-store feature flag is enabled
  2. Go through the onboarding task list and check that the copies and illustration match designs

Variations:

  • Shipping task only shows up for countries that don't have shipping smart defaults, e.g Afghanistan
  • WCPay only shows up in place of the regular payments task if WCPay is installed and activated
image image image image image image

Changelog entry

  • Automatically create a changelog entry from the details below.

Significance

  • Patch
  • Minor
  • Major

Type

  • Fix - Fixes an existing bug
  • Add - Adds functionality
  • Update - Update existing functionality
  • Dev - Development related task
  • Tweak - A minor adjustment to the codebase
  • Performance - Address performance issues
  • Enhancement - Improvement to existing functionality

Message

Comment

@rjchow rjchow marked this pull request as ready for review February 21, 2024 15:36
Copy link
Contributor

github-actions bot commented Feb 21, 2024

Test Results Summary

Commit SHA: 1006a29

Test 🧪Passed ✅Failed 🚨Broken 🚧Skipped ⏭️Unknown ❔Total 📊Duration ⏱️
API Tests25900202610m 37s
E2E Tests760027503516m 33s

To view the full API test report, click here.
To view the full E2E test report, click here.
To view all test reports, visit the WooCommerce Test Reports Dashboard.

@github-actions github-actions bot added plugin: woocommerce Issues related to the WooCommerce Core plugin. focus: e2e tests Issues related to e2e tests package: @woocommerce/e2e-core-tests Issues related to @woocommerce/e2e-core-tests package. package: @woocommerce/admin-e2e-tests issues related to @woocommerce/admin-e2e-tests labels Feb 22, 2024
@ilyasfoo ilyasfoo self-requested a review February 22, 2024 04:17
Copy link
Contributor

github-actions bot commented Feb 22, 2024

Hi @chihsuan, @adrianduffell, @woocommerce/ghidorah

Apart from reviewing the code changes, please make sure to review the testing instructions as well.

You can follow this guide to find out what good testing instructions should look like:
https://github.com/woocommerce/woocommerce/wiki/Writing-high-quality-testing-instructions

Copy link
Contributor

@ilyasfoo ilyasfoo left a comment

Choose a reason for hiding this comment

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

Shipping task only shows up for countries that don't have shipping smart defaults, e.g Afghanistan

I wasn't able to get the Shipping task to show when I chose Afghanistan via setup wizard. I used Hong Kong instead.

While reviewing, I noticed that we no longer have a way to display the "Import your products" variation of product task. I think it's because the condition here checks for selling_venues, which is deprecated since the new core profiler.

I think it makes sense to fix this bug along with updating the copies, since one of the use cases in Figma explicitly mentions it. I believe changing it to check business_choice == im_already_selling to show import variant is sufficient. What do you think?

Also, I don't see any changes regarding the WooPayments incentive screen (The Limited time offer copy). I'm not sure if it should be implemented in this PR since it might be obtained externally, but I think we'll need to communicate if we don't - perhaps WooPayments team can work on it?

@rjchow
Copy link
Contributor Author

rjchow commented Feb 22, 2024

Shipping task only shows up for countries that don't have shipping smart defaults, e.g Afghanistan

I wasn't able to get the Shipping task to show when I chose Afghanistan via setup wizard. I used Hong Kong instead.

Should have verified this, I was set to a country that worked and didn't check what it was 🙈

While reviewing, I noticed that we no longer have a way to display the "Import your products" variation of product task. I think it's because the condition here checks for selling_venues, which is deprecated since the new core profiler.

I think it makes sense to fix this bug along with updating the copies, since one of the use cases in Figma explicitly mentions it. I believe changing it to check business_choice == im_already_selling to show import variant is sufficient. What do you think?

Do need to fix this! I skipped it in the hurry last night and never got back to it. Thanks for catching that!

Also, I don't see any changes regarding the WooPayments incentive screen (The Limited time offer copy). I'm not sure if it should be implemented in this PR since it might be obtained externally, but I think we'll need to communicate if we don't - perhaps WooPayments team can work on it?

I couldn't find the text string in our repo I think, and I saw that it was being fetched from window so I skipped it. Good call to ping someone about it 😅

@rjchow rjchow force-pushed the add/update-task-list-copies-and-illustrations branch from b1c78ce to 590dece Compare March 7, 2024 13:34
@rjchow
Copy link
Contributor Author

rjchow commented Mar 7, 2024

  • Added change to remove the estimated times
  • Did not address change for the import product header as some PHP change will be required for the tasklist item text as well, which I'm not currently prepared to do and after discussion it's been okay-d to punt this to a later date
  • For the woo payments incentive copy, I traced the source down to https://github.com/Automattic/woocommerce-payments-server/pull/3499 and discussed it with the team over there - there's no release cycle constraint and all that's needed is to make a text change on L715

Copy link
Contributor

@ilyasfoo ilyasfoo left a comment

Choose a reason for hiding this comment

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

Thanks, @rjchow! Tests well!

Did not address change for the import product header as some PHP change will be required for the tasklist item text as well, which I'm not currently prepared to do and after discussion it's been okay-d to punt this to a later date

Sounds good, did you create a follow-up issue? I can help to create the issue if you want. Or do you plan to work on it after this is merged?

@rjchow
Copy link
Contributor Author

rjchow commented Mar 12, 2024

@ilyasfoo I can make the issue! I already actually did some work which I reverted cec152c because I realised the task list title text was mismatched

Edit: issue - #45500 , https://github.com/woocommerce/team-ghidorah/issues/292

@rjchow rjchow force-pushed the add/update-task-list-copies-and-illustrations branch from 91ee1e6 to 1006a29 Compare March 13, 2024 01:30
Copy link
Member

@chihsuan chihsuan left a comment

Choose a reason for hiding this comment

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

Thanks, lgtm and tested well. 👍

Copy link
Contributor

@adrianduffell adrianduffell left a comment

Choose a reason for hiding this comment

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

It tested great, @rjchow ! Thanks for adding the follow-up issues. Triple approval! 🚀

@rjchow rjchow merged commit 59635fb into trunk Mar 14, 2024
37 checks passed
@rjchow rjchow deleted the add/update-task-list-copies-and-illustrations branch March 14, 2024 07:20
@github-actions github-actions bot added this to the 8.8.0 milestone Mar 14, 2024
@github-actions github-actions bot added the needs: analysis Indicates if the PR requires a PR testing scrub session. label Mar 14, 2024
@veljkho veljkho added needs: external testing Indicates if the PR requires further testing conducted by testers external to the development team. status: analysis complete Indicates if a PR has been analysed by Solaris and removed needs: analysis Indicates if the PR requires a PR testing scrub session. labels Mar 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
focus: e2e tests Issues related to e2e tests needs: external testing Indicates if the PR requires further testing conducted by testers external to the development team. package: @woocommerce/admin-e2e-tests issues related to @woocommerce/admin-e2e-tests package: @woocommerce/e2e-core-tests Issues related to @woocommerce/e2e-core-tests package. plugin: woocommerce Issues related to the WooCommerce Core plugin. status: analysis complete Indicates if a PR has been analysed by Solaris
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants