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

My Home: Add "Connect social media accounts" Task #41018

Merged
merged 16 commits into from Apr 16, 2020

Conversation

Aurorum
Copy link
Contributor

@Aurorum Aurorum commented Apr 10, 2020

Changes proposed in this Pull Request

This creates a section for Recommended Tasks, and establishes the first one: "Connect social media accounts". I should note that I've tried to ensure that other tasks can easily be added within the same file though.

Testing instructions

Firstly, verify that this only displays if the site has no Publicize connections and when the checklist is hidden. The skip button should also dismiss the task, but not the card - other tasks should still appear. It'd probably be easier to demonstrate this with another task, but there's only one for now.

Screenshot 2020-04-11 at 10 05 45

When clicking the primary button, you should be redirected to Marketing. A Guided Tour should appear if you're on a desktop device.

Screenshot 2020-04-10 at 12 47 47

Issues:

  • This is a temporary illustration as I'm not sure how to access the one in the original issue (cc @danhauk)
  • The Guided Tour moves to the left of the screen before appearing. I'm not sure if this is an existing issue with other tours or how to fix this though

Part of #40734

@mmtr mmtr added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. [Feature] My Home The main dashboard for managing a WordPress.com site. labels Apr 10, 2020
@gwwar gwwar requested a review from a team April 10, 2020 21:45
@gwwar
Copy link
Contributor

gwwar commented Apr 10, 2020

Thanks for jumping into this one @Aurorum! 💖I'm going to ask folks some open questions to help think though next steps

  • @danhauk can we use the new illustration styles with colors? If not would it be possible to provide an image that we can use?
  • +1 for @Aurorum's suggestion of the Guided Tour, it makes a lot of sense ✨ How much guidance do folks think we should provide here @lcollette, for example should we walk through a full twitter connection? Maybe we should also provide a back to My Home link?
    Apr-10-2020 14-41-05
  • Fine to leave for the task container PR, but what should the secondary link "skip for now" do when there's only one task, when there's many tasks?
  • I spotted a couple of unrelated glitches from tours in general, but I'll file and link to them in a bit.

Aurorum and others added 2 commits April 13, 2020 12:19
Co-Authored-By: Miguel Torres <miguelmariatorresrojas@gmail.com>
@gwwar
Copy link
Contributor

gwwar commented Apr 15, 2020

I'll try and see if we can use the new ✨ illustrations for this one.

@mmtr mmtr changed the title Customer Home: Add Recommended Tasks MY Home: Add "Connect social media accounts" task Apr 16, 2020
@mmtr mmtr changed the title MY Home: Add "Connect social media accounts" task My Home: Add "Connect social media accounts" task Apr 16, 2020
@mmtr
Copy link
Member

mmtr commented Apr 16, 2020

Updated the PR title to reflect that this is focusing on the social connect task (so this is now part of #40734 rather than fixing it).

Copy link
Member

@mmtr mmtr left a comment

Choose a reason for hiding this comment

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

This works quite well for me, @Aurorum. Can we maybe avoid rendering the task card until we add it to the API?

Also noted the padding on the card is not homogenous (lower padding bottom than top).

Screen Shot 2020-04-16 at 11 40 56

I also agree with @gwwar above and think it'd be good if we link back to My Home when the guided tour is over. For reference, this how it looks on the site title update guided tour:

Screen Shot 2020-04-16 at 11 52 49

I'll try and see if we can use the new ✨ illustrations for this one.

I think we can handle that on a follow-up PR and land the card with a temporary illustration if we make sure the task card doesn't show up until the API returns it.

@Aurorum
Copy link
Contributor Author

Aurorum commented Apr 16, 2020

I added the Guided Tour but it looks like there's an issue with the z-index of the elements. Will explore it soon and resolve the font & other guided tour issue. :)

Screenshot 2020-04-16 at 12 26 22

@Aurorum
Copy link
Contributor Author

Aurorum commented Apr 16, 2020

Looks like the tests are failing on setup here too...not sure why 😕

@gwwar
Copy link
Contributor

gwwar commented Apr 16, 2020

I think Circle needs to roll back an update. We're seeing similar issues on other PRs

@gwwar
Copy link
Contributor

gwwar commented Apr 16, 2020

It may be worth rebasing @Aurorum with master, we've just switched over to yarn from npm. Let me know if you had any questions getting setup.

https://github.com/Automattic/wp-calypso#getting-started

@Aurorum Aurorum changed the title My Home: Add "Connect social media accounts" task My Home: Add "Connect social media accounts" Task Apr 16, 2020
@Aurorum
Copy link
Contributor Author

Aurorum commented Apr 16, 2020

Thanks @gwwar! I've managed to get it working with yarn.

I've fixed the issue with the support article dialog displaying beneath the guided tour, but I'm still struggling to add an extra step to the tour for when after a connection has been made. I've tried using <Continue when> and adding a selector in client/state/ui/guided-tours/contexts.js:

/**
 * Returns true if the selected site has any publicize connections.
 *
 * @param {object} state Global state tree
 * @returns {boolean} True if site has any publicize connections, false otherwise.
 */
export const doesSelectedSiteHavePublicizeConnections = state => {
	const siteId = getSelectedSiteId( state );
	if ( ! siteId ) {
		return false;
	}
	return getConnectionsBySiteId( state, siteId ).length > 0;
};

But no success with that, even after querying Publicize connections. Any ideas, or should I commit what I've got so far (albeit in a broken state)?

Copy link
Contributor

@gwwar gwwar left a comment

Choose a reason for hiding this comment

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

Thanks @Aurorum 💖

@gwwar
Copy link
Contributor

gwwar commented Apr 16, 2020

I'm not that familiar with guided tours but I think this is a good chunk to get in as is. We can also set this up so we can test this with the experimental layout flag that we added in #41145 to make this a little easier to iterate on.

@gwwar gwwar merged commit 40ded3e into Automattic:master Apr 16, 2020
@matticbot matticbot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Apr 16, 2020
) }
<div className="tasks__timing">
<Gridicon icon="time" size={ 18 } />
<p>{ translate( '3 minutes' ) }</p>
Copy link
Member

Choose a reason for hiding this comment

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

Could we please not hardcode values into translatable strings? This should be

translate( '%d minute', '%d minutes', { count: 3, arguments: [ 3 ] } )

like it's done in all the other places where we already do this, for example here: https://github.com/Automattic/wp-calypso/blob/master/client/my-sites/customer-home/cards/tasks/checklist-site-setup/wpcom-checklist/component.jsx#L263

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops, thank you! I opened #41271 to address that. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] My Home The main dashboard for managing a WordPress.com site. OSS Citizen
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants