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

Added "Finish store setup" card to My Home #44547

Merged
merged 12 commits into from
Sep 16, 2020
Merged

Conversation

octaedro
Copy link
Contributor

@octaedro octaedro commented Jul 29, 2020

Changes proposed in this Pull Request

  • This PR is part of a bigger change related to the WC-Admin issue 554.

  • It focuses on only the visual changes.

  • This PR adds the "Finish store setup" card to My Home.

screenshot-calypso localhost_3000-2020 09 16-16_33_31
9fdeede5d7.png)

Testing instructions

In order to test this PR, these changes should be approved in Phabricator first.

  • Choose one page and go to My Home, adding the params?dev=true&view=VIEW_SITE_SETUP_ECOMMERCE to the URL (it will look like http://calypso.localhost:3000/home/[my_site]?dev=true&view=VIEW_SITE_SETUP_ECOMMERCE).
  • Verify that the Finish store setup card is visible.
  • Verify that the Quick link named Finish store setup is visible too.
  • The button Finish store setup should be pointing to the site's store.
  • Verify that the description text and the image in the card are correct.

Fixes partially 554

@octaedro octaedro requested a review from a team as a code owner July 29, 2020 22:19
@octaedro octaedro self-assigned this Jul 29, 2020
@matticbot
Copy link
Contributor

@matticbot matticbot added the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Jul 29, 2020
@octaedro octaedro removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Jul 29, 2020
@octaedro octaedro removed the request for review from a team July 29, 2020 22:19
@matticbot
Copy link
Contributor

matticbot commented Jul 29, 2020

Here is how your PR affects size of JS and CSS bundles shipped to the user's browser:

Sections (~303 bytes added 📈 [gzipped])

name  parsed_size           gzip_size
home      +1162 B  (+0.3%)     +303 B  (+0.3%)

Sections contain code specific for a given set of routes. Is downloaded and parsed only when a particular route is navigated to.

Legend

What is parsed and gzip size?

Parsed Size: Uncompressed size of the JS and CSS files. This much code needs to be parsed and stored in memory.
Gzip Size: Compressed size of the JS and CSS files. This much data needs to be downloaded over network.

Generated by performance advisor bot at iscalypsofastyet.com.

@gwwar gwwar requested a review from a team July 29, 2020 22:21
@matticbot matticbot added the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Jul 29, 2020
* @param {number} siteId Site ID
* @returns {object} Object with store setup information
*/
export default function getStoreSetup( state, siteId ) {
Copy link
Contributor

@gwwar gwwar Jul 29, 2020

Choose a reason for hiding this comment

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

Please handle any view checks on the server, see PCYsg-qqr-p2 or PCYsg-qpk-p2 for more info. If you have any questions @Automattic/serenity can help walk you through this.

Copy link
Contributor

@gwwar gwwar Jul 29, 2020

Choose a reason for hiding this comment

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

Totally fine though to let the task component query if we need to display remaining tasks left.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for your comments @gwwar, the documentation you shared is very helpful.
In this case, as you mentioned here, I should add an endpoint in Jetpack to expose the information I need, am I right?
Also, just to confirm, (please correct me if I'm missing something) in this repo I should create a structure similar to what we use with the checklist, to handle the data we get from Jetpack, yes?

Copy link
Contributor

@kwight kwight Aug 4, 2020

Choose a reason for hiding this comment

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

I should add an endpoint in Jetpack to expose the information I need, am I right?

Correct!

in this repo I should create a structure similar to what we use with the checklist, to handle the data we get from Jetpack, yes?

I'm not sure I follow what's going on there, but I believe it's a checklist for use outside of of My Home, so likely more complicated than you need. I'd say a better model in your case might be the ConnectAccountsTask component. Your task component would have a query component that fetches the data from your new endpoint, and updates appropriately. So the overall idea is:

  • My Home page load calls the home/layout endpoint.
  • Server determines which view to send back; let's say your view in this case.
  • The API returns the list of cards to show (which has your TASK_SITE_SETUP_CHECKLIST_ECOMMERCE in the main spot).
  • The page renders your card/task, which triggers the query component to fetch the checklist data you require.
  • When that comes back, the task is visually updated as required.
  • As a user completes tasks, your checklist data should remain your "single source of truth", so that any fresh page reload of My Home would go through the above again and render the correct completion data.

Copy link
Contributor Author

@octaedro octaedro Aug 6, 2020

Choose a reason for hiding this comment

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

Thank you @kwight, now I get it 🙂

@octaedro octaedro removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Jul 29, 2020
@kwight
Copy link
Contributor

kwight commented Aug 4, 2020

To test this PR is necessary to add a HomeLayout and StoreSetup data. I added to the branch add/store-setup-on-my-home_test.

We also have dev mode for My Home views, explained in PCYsg-qqj-p2 – this makes it easier to force a certain view while you're working (but remember you do need to add the view to your sandbox). At least with this approach, you can be testing the actual branch you'll be merging, rather than a modified substitute.

@sixhours sixhours requested a review from a team August 5, 2020 13:33
@matticbot matticbot added the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Aug 5, 2020
@sixhours
Copy link
Contributor

sixhours commented Aug 5, 2020

👋 Just catching up here.

I'm wondering if the "scary" button is too much for this scenario. We typically associate red with "This is very, very broken and needs immediate attention." It could create a sense of panic or alarm in the user. If the primary task card in My Home is "What should I do next?" I would think the regular primary CTA would be enough emphasis without being alarming. What do y'all think?

Cc'ing @Automattic/dotcom-manage-design in case they have thoughts.

@olesyabrk
Copy link
Contributor

@sixhours, I 100% agree with you. Is there a reason we're not using our regular primary button here?

@octaedro octaedro removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Aug 6, 2020
@octaedro
Copy link
Contributor Author

octaedro commented Aug 6, 2020

I'm wondering if the "scary" button is too much for this scenario. We typically associate red with "This is very, very broken and needs immediate attention." It could create a sense of panic or alarm in the user. If the primary task card in My Home is "What should I do next?" I would think the regular primary CTA would be enough emphasis without being alarming. What do y'all think?

Thank you @sixhours and @olesyabrk for the advice. Good idea, I will follow the pattern that is already used here and leave that button as primary.

/cc: @jameskoster

@matticbot matticbot added the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Aug 6, 2020
@kwight
Copy link
Contributor

kwight commented Aug 27, 2020

Is this still active @octaedro ?

@octaedro
Copy link
Contributor Author

Is this still active @octaedro ?

Hi @kwight,
I just finished another issue and now I'm focused on wrapping up this PR 🙂

Fernando Marichal added 4 commits September 4, 2020 11:16
This commit adds the "Finish store setup" card to My home
This commit changes the component "FinishStoreSetup" name and decouples it from "task.jsx"
This commit adds a query component and the code necessary to handle site-setup-ecommerce store
Given some difficulties we currently have on the wc-admin side to get the params we need, the card was modified to not to use them for now.
@octaedro octaedro removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Sep 4, 2020
@matticbot matticbot added the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Sep 7, 2020
@octaedro octaedro removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Sep 7, 2020
Copy link
Contributor

@timmyc timmyc left a comment

Choose a reason for hiding this comment

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

Tested this out with D49062-code and it is working really well Fernando!

@kwight or @gwwar - since this is our first foray into the server-side homepage views logic, would one of you be able to give the phabricator diff a look too? 🙇

@matticbot matticbot added the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Sep 7, 2020
@kwight
Copy link
Contributor

kwight commented Sep 8, 2020

since this is our first foray into the server-side homepage views logic, would one of you be able to give the phabricator diff a look too?

Sure, it's on Serenity's board, we should get to it today 👍

@kwight
Copy link
Contributor

kwight commented Sep 8, 2020

Can we not add the link to the Quick Links section? We wanted to keep that tight and generic, with nothing specific to individual tasks (they would pile up and loose effectiveness pretty quickly). cc/ @sixhours if I'm off the mark here.

const translate = useTranslate();

return (
<Task
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you want to add the timing prop, to give users an idea of how long the task would take?

Screen Shot 2020-09-08 at 11 42 30 AM

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for the suggestion @kwight, I like the idea but I'm not sure about how much time to show there, since the task list has a few tasks and we're not able to know how many the user needs to finish. If it's only one, they could finish it in a minute or two but if they have all of them undone for sure it will take more time.

Copy link
Contributor

Choose a reason for hiding this comment

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

We don't aim for accuracy – it's more like "hey, it'll probably take you about 10 minutes if you do it all right now". But certainly up to you if you thinking dropping it is the way to go.

Copy link
Contributor Author

@octaedro octaedro Sep 11, 2020

Choose a reason for hiding this comment

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

Hi @pmcpinto,
What do you think about the estimated time for the Finish store setup card?
Is it ok if we add a fixed 7 minutes to this task?

Choose a reason for hiding this comment

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

@octaedro 7 min sounds good, as it's the sum of the estimated time of the main tasks (products, shipping, taxes, payments, customize store)

Copy link
Contributor Author

@octaedro octaedro Sep 15, 2020

Choose a reason for hiding this comment

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

Added the timer prop with 7 minutes in the commit b8235d4

@sixhours
Copy link
Contributor

sixhours commented Sep 8, 2020

Can we not add the link to the Quick Links section? We wanted to keep that tight and generic, with nothing specific to individual tasks (they would pile up and loose effectiveness pretty quickly). cc/ @sixhours if I'm off the mark here.

Good catch! Since this step is featured in the banner/header area, it's very prominent, so adding it to Quick Links creates extra noise.

Once someone has finished the store setup, I could see adding a Quick Link to "Add new product" or similar, perhaps. The store setup is a transient step, whereas Quick Links should be stuff that's going to be used again and again.

Let me know if that makes sense!

@octaedro
Copy link
Contributor Author

octaedro commented Sep 9, 2020

Can we not add the link to the Quick Links section? We wanted to keep that tight and generic, with nothing specific to individual tasks (they would pile up and loose effectiveness pretty quickly). cc/ @sixhours if I'm off the mark here.

Good catch! Since this step is featured in the banner/header area, it's very prominent, so adding it to Quick Links creates extra noise.

Once someone has finished the store setup, I could see adding a Quick Link to "Add new product" or similar, perhaps. The store setup is a transient step, whereas Quick Links should be stuff that's going to be used again and again.

Let me know if that makes sense!

Thank you for the feedback, it sounds good to me. Just to confirm I will CC @jameskoster

@kwight
Copy link
Contributor

kwight commented Sep 10, 2020

I tested this with D49062-code, it's looking good to me 🎉

This commit removes 'Finish store setup' from quick links
@octaedro
Copy link
Contributor Author

I removed the Finish store setup option form the Quick Links in the commit 67ff5cf.

@octaedro
Copy link
Contributor Author

Hi everybody,
In order to wrap this PR up, I need an image or illustration for this card, to maintain some visual interest.
screenshot-calypso localhost_3000-2020 09 13-11_02_42

Could somebody point me towards the right person to ask for this image?
cc @danhauk @sixhours @kwight

@kwight
Copy link
Contributor

kwight commented Sep 14, 2020

There's https://github.com/Automattic/wp-calypso/tree/master/client/assets/images with images, including the customer-home subfolder for specific My Home images. For creating new ones of that most recent style, I think it was @danhauk (who is no longer working on dotcom) – @sixhours are these illustrations something you can generate?

@octaedro at one time, there was a speedometer illustration, is that no longer applicable or usable?

@octaedro
Copy link
Contributor Author

@octaedro at one time, there was a speedometer illustration, is that no longer applicable or usable?

Hi @kwight,
In the original design we had a progress bar, but since we aren't able to send the params we need without a pretty big refactor, we decided to not use it.
This is why we need an image to add there in order to maintain some visual interest.

This commit adds timing prop to the `Finish store setup` card
@sixhours
Copy link
Contributor

@sixhours are these illustrations something you can generate?

I can! I'll sync up with Dan and see how he did those. I'm not sure I'd be able to get to it until next week, though. In the meantime, would one of our existing illustrations from that "family" work? Maybe the Payments coin illustration? https://wordpress.com/calypso/images/earn-section-3e8fbe66663516cbb6bdc5037be7b643.svg

This commit adds the `earnSection` image to the card
@octaedro
Copy link
Contributor Author

I can! I'll sync up with Dan and see how he did those. I'm not sure I'd be able to get to it until next week, though. In the meantime, would one of our existing illustrations from that "family" work? Maybe the Payments coin illustration? https://wordpress.com/calypso/images/earn-section-3e8fbe66663516cbb6bdc5037be7b643.svg

Thank you @sixhours, I added the image you suggested in the commit 7d5b15b and it looks good 🙂
Please let me know when you have the new image ready.

@kwight
Copy link
Contributor

kwight commented Sep 15, 2020

Looks great to me:

Screen Shot 2020-09-15 at 10 54 20 AM

As mentioned by @mmtr , be sure to land this PR first before D49062-code to avoid Calypso errors.

@octaedro octaedro merged commit 71517e6 into master Sep 16, 2020
@octaedro octaedro deleted the add/store-setup-on-my-home branch September 16, 2020 16:54
@matticbot matticbot removed [Status] In Progress [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Sep 16, 2020
@a8ci18n
Copy link

a8ci18n commented Sep 16, 2020

This Pull Request is now available for translation here: https://translate.wordpress.com/deliverables/4425576

Thank you @octaedro for including a screenshot in the description! This is really helpful for our translators.

@a8ci18n
Copy link

a8ci18n commented Sep 25, 2020

Translation for this Pull Request has now been finished.

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

Successfully merging this pull request may close these issues.

None yet

9 participants