Skip to content

Fix progress header title #32786

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.

Sign up for GitHub

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

Jump to bottom
Merged
octaedro merged 4 commits into from
Apr 29, 2022
Merged

Fix progress header title #32786

octaedro merged 4 commits into from
Apr 29, 2022

Conversation

@octaedro
Copy link
Contributor

@octaedro octaedro commented Apr 26, 2022 •
edited
Loading

All Submissions:

Changes proposed in this Pull Request:

This PR fixes the progress header title behavior.

The progress header title Welcome to <store_name> will be visible only when any of these conditions is true:

  • The user doesn't have any completed tasks
  • After completing every task
  • The user completed only the OBW

(more details here)

If we don't know the store's name, it will say Welcome to your store (more details here).

Closes #32720.

How to test the changes in this Pull Request:

  1. Start with a fresh install
  2. Install and activate the latest WooCommerce Test Helper plugin
  3. Navigate to Tools > WCA Test Helper > Experiments and toggle woocommerce_tasklist_setup_experiment_2_2022_* to treatment.
  4. Navigate to WooCommerce > Home, start the onboarding wizard, and press skip.
  5. The progress header should show up with a Welcome to <site name> above the task list.
  6. Now start and finish the OBW.
  7. The progress header should stay the same,
  8. Finish another task.
  9. Now the progress header should say Let's get you started :rocket:.
  10. Finish all the tasks.
  11. Now the progress header should show: Welcome to <site name>.

Other information:

  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes, as applicable?
  • Have you successfully run tests with your changes locally?
  • Have you created a changelog file by running pnpm nx affected --target=changelog?

FOR PR REVIEWER ONLY:

  • I have reviewed that everything is sanitized/escaped appropriately for any SQL or XSS injection possibilities. I made sure Linting is not ignored or disabled.

@github-actions github-actions bot added focus: react admin plugin: woocommerce Issues related to the WooCommerce Core plugin. labels Apr 26, 2022
@octaedro octaedro marked this pull request as ready for review April 26, 2022 19:24
@octaedro octaedro requested a review from a team April 27, 2022 17:46
@louwie17 louwie17 requested review from louwie17 and removed request for a team April 28, 2022 17:22
louwie17
louwie17 requested changes Apr 28, 2022
View reviewed changes
Copy link
Contributor

@louwie17 louwie17 left a comment

Choose a reason for hiding this comment

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

@octaedro I tried to follow the testing instructions, but I always get Let's get you started 🚀 for the first several steps and I don't see Welcome to <store name> anymore, only at the end once I completed all the tasks.
I left a comment inline as well.

plugins/woocommerce-admin/client/task-lists/progress-header/progress-header.tsx Outdated
completedCount === tasksCount
completedCount === 0 ||
completedCount === tasksCount ||
( hasFinishedStoreDetails && completedCount === 1 )
Copy link
Contributor

@louwie17 louwie17 Apr 28, 2022

Choose a reason for hiding this comment

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

This logic doesn't seem to work for me. It always shows Let's get you started now at the beginning. Because either the finishedStoreDetails is false and completedCount === 1 or finishedStoreDetails is true and completedCount === 2. Neither of these work for the above condition.

As a simpler fix, could we just filter out the store_details task from the previous hasVisitedTasks logic?

Copy link
Contributor Author

@octaedro octaedro Apr 29, 2022

Choose a reason for hiding this comment

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

Yes, this is pretty confusing. Sorry for not having been more explicit in my instructions.
The title Welcome to <store name> won't be visible if any task (other than the Store Details) has been finished.
If completedCount is 1 but the Store Details task is incomplete, the title shouldn't be visible and we will see Let's get you started 🚀 instead. Same if there is more than one task completed.

On the other hand, although the functionality is almost the same, the change you mentioned makes the code easier to read so I added it.

plugins/woocommerce-admin/client/task-lists/progress-header/progress-header.tsx
siteTitle
)
: __( 'Welcome', 'woocommerce' );
: __( 'Welcome to your store', 'woocommerce' );
Copy link
Contributor

@louwie17 louwie17 Apr 28, 2022

Choose a reason for hiding this comment

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

Nice!

@louwie17 louwie17 added the needs: author feedback The issue/PR needs a response from any of the parties involved in the issue. label Apr 28, 2022
@octaedro octaedro force-pushed the fix/32720_progress-header-title branch from 0a1916e to b8a8e9d Compare April 29, 2022 16:48
@octaedro
Copy link
Contributor Author

octaedro commented Apr 29, 2022

Thank you @louwie17 for the review. I addressed the change you suggested. Could you take another look at this PR?

@octaedro octaedro requested a review from louwie17 April 29, 2022 17:28
@github-actions github-actions bot added needs: triage feedback Issues for which we requested feedback from the author and received it. and removed needs: author feedback The issue/PR needs a response from any of the parties involved in the issue. labels Apr 29, 2022
louwie17
louwie17 reviewed Apr 29, 2022
View reviewed changes
Copy link
Contributor

@louwie17 louwie17 left a comment

Choose a reason for hiding this comment

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

@octaedro thanks for the changes, this test great now for the task list experiment 1, but not for task experiment 2, which I will take the blame for, as it looks like a bug that I caused. It doesn't update the visitedTasks logic in the sectioned task list.

Would you potentially be able to add that logic as part of this PR, we did need to add most of these lines (with the update in the trackClick function) https://github.com/woocommerce/woocommerce/blob/trunk/plugins/woocommerce-admin/client/tasks/task-list-item.tsx#L108-L130
To this component: https://github.com/woocommerce/woocommerce/blob/trunk/plugins/woocommerce-admin/client/two-column-tasks/task-list-item.tsx#L41

@octaedro
Copy link
Contributor Author

octaedro commented Apr 29, 2022

@louwie17 I added the fix you suggested in the commit 23c829e
I think that this PR is ready for another review 🙂

@octaedro octaedro requested a review from louwie17 April 29, 2022 19:32
louwie17
louwie17 approved these changes Apr 29, 2022
View reviewed changes
Copy link
Contributor

@louwie17 louwie17 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 sticking with my suggested changes @octaedro, this tested great and changes look good! Thanks for fixing this 🚀

@octaedro octaedro merged commit c60884e into trunk Apr 29, 2022
@octaedro octaedro deleted the fix/32720_progress-header-title branch April 29, 2022 21:56
@github-actions github-actions bot added this to the 6.6.0 milestone Apr 29, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Apr 29, 2022

Hi @octaedro, thanks for merging this pull request. Please take a look at these follow-up tasks you may need to perform:

  • Add the release: add testing instructions label

@github-actions github-actions bot mentioned this pull request Jun 15, 2022
Closed
This was referenced Jul 4, 2022
Closed
Closed
Closed
Closed
@github-actions github-actions bot mentioned this pull request Jul 8, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@louwie17 louwie17 louwie17 approved these changes

Assignees

No one assigned

Labels

needs: triage feedback Issues for which we requested feedback from the author and received it. plugin: woocommerce Issues related to the WooCommerce Core plugin.

Projects

None yet

Milestone

6.6.0

Development

Successfully merging this pull request may close these issues.

[GlobalStep] Unable to display "Welcome to <site name>" above the progress header task list on "Woocommerce > Home" page.

3 participants

@octaedro @louwie17