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: store location settings tour #34137

Merged
merged 1 commit into from
Aug 2, 2022

Conversation

rjchow
Copy link
Contributor

@rjchow rjchow commented Aug 1, 2022

All Submissions:

Changes proposed in this Pull Request:

Part 2 of #32587, to add spotlight to the store location settings.

The spotlight should only show up when visited from the task list, and it should only show up once

Closes #32587 .

How to test the changes in this Pull Request:

  1. Start with a fresh install of WooCommerce, optionally complete OBW or not (not a condition to have completed it)
  2. Go to the task list and click on "You added store details" or "Add store details" in the tasklist (it might be struck through)
  3. You should be redirected to the WooCommerce settings page, on the general tab and have a spotlight on the store location fields.
  4. Subsequent visits should not show the spotlight

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 for each project being changed, ie pnpm changelog add --filter=<project>?

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 package: @woocommerce/components issues related to @woocommerce/components plugin: woocommerce Issues related to the WooCommerce Core plugin. labels Aug 1, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Aug 1, 2022

Test Results Summary

Commit SHA: 4555221

Test 🧪Passed ✅Failed 🚨Broken 🚧Skipped ⏭️Unknown ❔Total 📊Duration ⏱️
API Tests11500201170m 40s
E2E Tests185001018613m 38s
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.

@rjchow rjchow marked this pull request as ready for review August 1, 2022 08:16
@rjchow rjchow requested a review from a team August 1, 2022 08:16
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.

So far this is looking good, @rjchow!

As we're growing the use case for spotlight, I'm wondering if we need to consolidate "viewed" state of different spotlights so it can become easier to manage. Either that or perhaps we can have a standard naming convention so we can easily search for all spotlight from an option prefix.

Also, I think we may want to add tracking for this? As our product spotlight, we had a "view", "complete", "dismiss". I'm not sure if we should also monitor if users fill up the store address when they click on complete since this is a single step (user may accidentally dismiss without being able to get the tour back). cc @verofasulo @SamratBiswas1

Though I'd also be happy if the track is to be addressed in a follow up!

@verofasulo
Copy link

I'm not sure if we should also monitor if users fill up the store address when they click on complete since this is a single step

Excellent point @ilyasfoo - I think we should as that's the point of the task :) It's not about clicking on the task in the task list but evaluating how effective it is (do users complete the store location details?).

@rjchow
Copy link
Contributor Author

rjchow commented Aug 1, 2022

Yea I agree we need to do something about the spotlight sprawl, so far we have two options being used and I think we probably could consolidate it under one. Maybe as stringified json, along with a useHook - have some ideas around that. But not in this PR 😁

Ah ok I didn't expect to need tracks here but ok sure thing I'll put up an issue for it!

@ilyasfoo ilyasfoo self-requested a review August 2, 2022 02:38
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 for confirming, @verofasulo!

Sounds good, @rjchow! This tested well, great work! LGTM! 🚢

@rjchow rjchow merged commit cb09649 into trunk Aug 2, 2022
@rjchow rjchow deleted the add/store-location-settings-spotlight-tour branch August 2, 2022 03:10
@github-actions github-actions bot added this to the 6.9.0 milestone Aug 2, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Aug 2, 2022

Hi @rjchow, 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

@rjchow
Copy link
Contributor Author

rjchow commented Aug 8, 2022

@verofasulo I just noticed a UX issue with this store location tour while running through the flow, so far the tour only highlights the fields that need to be changed.

My natural instinct was to fill them out and ... forget to scroll to the bottom to click save - instead I clicked on the next step button.

I think the ideal UX would be to autosave for the user on completion of the tour, if they have populated the fields. However I'm not sure its possible, and if it is it's not straightforward. So I think we might need to add a second step to prompt the user to save?

@rjchow
Copy link
Contributor Author

rjchow commented Aug 8, 2022

@ilyasfoo regarding tracks I think we might not need to add anything,

I was looking in the code and found that we have tracks for task completion. So we just need to predicate it on visitation of the settings page via the task list

Looking in the tracks log from the WooCommerce status page:

2022-08-08T12:00:13+00:00 DEBUG wcadmin_settings_change
2022-08-08T12:00:13+00:00 DEBUG   - settings: woocommerce_store_address,woocommerce_store_address_2,woocommerce_store_city,woocommerce_store_postcode,woocommerce_all_except_countries,woocommerce_specific_allowed_countries,woocommerce_specific_ship_to_countries
2022-08-08T12:00:13+00:00 DEBUG   - tab: general
2022-08-08T12:00:13+00:00 DEBUG   - section: 
2022-08-08T12:00:14+00:00 DEBUG wcadmin_settings_view
2022-08-08T12:00:14+00:00 DEBUG   - tab: general
2022-08-08T12:00:14+00:00 DEBUG   - section: 
2022-08-08T12:00:14+00:00 DEBUG 
2022-08-08T12:00:14+00:00 DEBUG   - _via_ua: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:103.0) Gecko/20100101 Firefox/103.0
2022-08-08T12:00:14+00:00 DEBUG   - _via_ip: 192.168.16.1
2022-08-08T12:00:14+00:00 DEBUG   - _lg: en-US,en;q=0.5
2022-08-08T12:00:14+00:00 DEBUG   - _dr: http://localhost:8888/wp-admin/admin.php?page=wc-settings&tab=general&tutorial=true
2022-08-08T12:00:14+00:00 DEBUG   - _dl: http://localhost:8888/wp-admin/admin.php?page=wc-settings&tab=general&tutorial=true
2022-08-08T12:00:14+00:00 DEBUG   - url: http://localhost:8888
2022-08-08T12:00:14+00:00 DEBUG   - blog_lang: en_US
2022-08-08T12:00:14+00:00 DEBUG   - blog_id: 
2022-08-08T12:00:14+00:00 DEBUG   - products_count: 0
2022-08-08T12:00:14+00:00 DEBUG   - wc_version: 6.9.0
2022-08-08T12:00:16+00:00 DEBUG wcadmin_tasklist_task_completed
2022-08-08T12:00:16+00:00 DEBUG   - task_name: store_details

And in the tracks log on the browser side:

wc-admin:tracks recordevent wcadmin_page_view 
Object { path: "/wp-admin/admin.php?page=wc-settings&tab=general&tutorial=true", is_embedded: true, has_navigation: true }
 +0ms 
Object { _tqk: {…}, shouldRecord: false }
index.js:313:13

Do you think that's ok? I couldn't find a server side codepath that catches the user clicking on the tasklist item

@verofasulo
Copy link

My natural instinct was to fill them out and ... forget to scroll to the bottom to click save - instead I clicked on the next step button.

@rjchow could you please clarify what the next step button is?

Generally speaking, this is a well-known UX issue 😅 The user needs to navigate down till the end of the page to save changes and this is not ideal. @jarekmorawski has been working on a new header component that can hopefully be applied to the Settings section as well and should help solve this issue 😊

Right now, I would say not to overcomplicate the experience by adding a new step and just leave it as it till the new component is ready to be implemented.

@rjchow
Copy link
Contributor Author

rjchow commented Aug 10, 2022

My natural instinct was to fill them out and ... forget to scroll to the bottom to click save - instead I clicked on the next step button.

@rjchow could you please clarify what the next step button is?

Generally speaking, this is a well-known UX issue 😅 The user needs to navigate down till the end of the page to save changes and this is not ideal. @jarekmorawski has been working on a new header component that can hopefully be applied to the Settings section as well and should help solve this issue 😊

Right now, I would say not to overcomplicate the experience by adding a new step and just leave it as it till the new component is ready to be implemented.

@verofasulo sorry I meant the "Done!" button on the tour message frame

@verofasulo
Copy link

sorry I meant the "Done!" button on the tour message frame

Do you mean the "Got it" button, right?

image

@rjchow
Copy link
Contributor Author

rjchow commented Aug 10, 2022

Sorry yes 😅 I misremembered the exact wording but that’s what it is

@ilyasfoo
Copy link
Contributor

ilyasfoo commented Aug 11, 2022

Do you think that's ok? I couldn't find a server side codepath that catches the user clicking on the tasklist item

@rjchow I think it's not as accurate. For example, the wcadmin_page_view track does not properly exclude users on subsequent visits which may be significant. wcadmin_settings_change could work for assuming completion though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package: @woocommerce/components issues related to @woocommerce/components plugin: woocommerce Issues related to the WooCommerce Core plugin.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Store location: Task list
3 participants