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 purchase task to experimental task lists #33178
Conversation
📊 Test reports for this pull request have been published and are accessible through the following links:
Latest commit referenced in the reports: Visit the WooCommerce Test Reports homepage to view all published reports. See the FAQs page if you're having problems accessing them. |
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.
Overall tested well, but left some comments in relation to styling and copy that should match the design of the task list. I think the copy change mentioned at the end here, would be the biggest item, we could do without the header (or do that as part of a separate PR).
For experiment 1 we have headers for each task, but don't have a header for this one, I think we would want to create one.
vs
the header of ways to get paid:
@jarekmorawski might have some suggestions?
Also we want to update the titles to match the copy of the new task list.
For example for both experiments, the completed copy says:
'Add paid extensions to my store' but all the others are in past tense, maybe we should change it to 'You added paid extensions to your store'?
See an example of the logic here: https://github.com/woocommerce/woocommerce/blob/trunk/plugins/woocommerce/src/Admin/Features/OnboardingTasks/Tasks/Products.php#L43-L49
Absolutely! I added the updated designs here: G9vi4e286yjdzyIdg8hBb1-fi-1761%3A159201 Here's how the copy would go: Task title
Description
For the experiment 1 (single task list), the button should say |
Thanks for all the input and help on this, @jarekmorawski! @louwie17 I've updated based on the discussion in p1653660464272079-slack. Also updated the width/height of illustrations in 4044869 which needs a sanity check, but I think will confine proportions well. |
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.
Thanks for the updates Josh, the header looks great, I did notice a minor issue with the CSS changes.
I also left some comments in relation to the action button and the completed copy.
Also sorry if some of these comments show up twice, Github is being annoying....
$additional_count = count( $products['remaining'] ) - 1; | ||
|
||
if ( $this->get_parent_option( 'use_completed_title' ) && $this->is_complete() ) { | ||
return count( $products['remaining'] ) === 1 |
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.
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.
Same probably goes for $addtional_count and line 79.
For additional_count probably need a $completed_additional_count
.
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.
Ya, good point. This doesn't work well as you complete purchases. Updated.
$additional_count, | ||
'woocommerce' | ||
), | ||
$products['remaining'][0], |
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.
Also in the $products['purchaseable'] the items are array's of plugins, so this will probably be $products['purchaseable'][0]['label']
<Button | ||
isSecondary={ task.isComplete } | ||
isPrimary={ ! task.isComplete } | ||
onClick={ goToTask } |
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.
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 think ultimately if we keep this variant of the task list we should strive to dry up these headers and actions a bit. There is a lot of duplication between components and even backend data being replicated on the frontend that is going to cause headaches down the road.
<Button | ||
isSecondary={ task.isComplete } | ||
isPrimary={ ! task.isComplete } | ||
onClick={ goToTask } |
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.
See the custom purchase task item we have here: https://github.com/woocommerce/woocommerce/blob/trunk/plugins/woocommerce-admin/client/tasks/fills/purchase.js
I know for other headers like WooCommerce Payments we copied that custom logic to the header also instead of using goToTask
-> https://github.com/woocommerce/woocommerce/blob/trunk/plugins/woocommerce-admin/client/two-column-tasks/headers/woocommerce-payments.js#L86
img { | ||
max-height: 150px; | ||
width: auto; | ||
} |
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.
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.
Add max width and max height. Seems to be working well on my end, can you double check on yours?
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.
Looks good on my end now to :)
@joshuatf you also have one PHP test failing: https://github.com/woocommerce/woocommerce/runs/6659717571?check_suite_focus=true |
Hey @louwie17, updated that old test and added another for better coverage around titles. Could you give this another look? |
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.
Thanks for the updated changes @joshuatf , nice work on this, this tested well and changes look great 👍 💯
Hi @joshuatf, thanks for merging this pull request. Please take a look at these follow-up tasks you may need to perform:
|
All Submissions:
Changes proposed in this Pull Request:
Adds the missing purchase task to the 2 experimental task lists.
Closes #33125 .
How to test the changes in this Pull Request:
woocommerce_tasklist_setup_experiment_1_*
woocommerce_tasklist_setup_experiment_2_*
experimentOther information:
pnpm nx changelog <project>
?FOR PR REVIEWER ONLY: