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.

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

AnchorFM: Podcasting Card #47487

Merged
merged 5 commits into from
Nov 19, 2020
Merged

AnchorFM: Podcasting Card #47487

merged 5 commits into from
Nov 19, 2020

Conversation

krymson24
Copy link
Contributor

@krymson24 krymson24 commented Nov 17, 2020 •
edited
Loading

Changes proposed in this Pull Request

  • Added a task card for starting a podcast website with Anchor.fm

Testing instructions

  1. Sandbox the API and apply this diff D52847-code
  2. Uncomment line 54 from wp-content/lib/home/views.php
  3. Use Calypso Live to checkout this branch: AnchorFM: Podcasting Card #47487
  4. On your site https://wordpress.com/home/{site}, where you've already gone through the introductory set-up tasks, dismiss the cards at the top until you see the podcast card.
  5. Click on "Get Started". This should take you to https://anchor.fm/. Clicking on "Hide this" should dismiss this card.

Screenshots and GIFs

Screen Shot 2020-11-16 at 7 29 10 PM

2020-11-16 19 29 15

2020-11-16 19 29 28

Fixes 310-gh-dotcom-manage

@krymson24 krymson24 added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. Podcasting labels Nov 17, 2020
@krymson24 krymson24 requested a review from a team November 17, 2020 02:39
@krymson24 krymson24 requested a review from a team as a code owner November 17, 2020 02:39
@krymson24 krymson24 self-assigned this Nov 17, 2020
@matticbot
Copy link
Contributor

Test live: https://calypso.live/?branch=anchorfm-card

@matticbot
Copy link
Contributor

matticbot commented Nov 17, 2020 •
edited
Loading

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

Sections (~124 bytes added 📈 [gzipped])

name  parsed_size           gzip_size
home       +412 B  (+0.1%)     +124 B  (+0.1%)

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.

mmtr
mmtr reviewed Nov 17, 2020
View reviewed changes
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.

Sandbox the API and apply this diff.

I guess this refers to D52847-code?

client/my-sites/customer-home/cards/tasks/podcasting/index.jsx
`Easily turn your blog into a podcast with Anchor — the world's biggest podcasting platform.`
) }
actionText={ translate( 'Get started' ) }
actionUrl={ `https://anchor.fm` }
Copy link
Member

Choose a reason for hiding this comment

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

Is this a temporary link? Similarly to the Stripe connection process on Earn blocks, I think this should redirect to a URL where users can connect their Anchor.fm account to their WP.com account and then they are redirected back to WP.com. But probably that process is still not implemented, in that case the temporary link is fine, but maybe we can include a TODO comment linking to the pending issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I followed the description here: 310-gh-dotcom-manage

I agree -- the action URL as it stands doesn't make sense to me. I'll add a TODO

@sixhours
Copy link
Contributor

I took this for a quick spin and it worked! 👍

mmtr
mmtr approved these changes Nov 18, 2020
View reviewed changes
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.

Great start! As noted in D52847-code and in the comments of this PR, I think we still need to apply to some iterations, but it can be followed up later (we can create new issues or keep 310-gh-Automattic/dotcom-manage opened and note there the pending stuff).

client/my-sites/customer-home/cards/tasks/podcasting/index.jsx Outdated
`Easily turn your blog into a podcast with Anchor — the world's biggest podcasting platform.`
) }
actionText={ translate( 'Get started' ) }
// TODO replace with more appropriate URL: https://github.com/Automattic/dotcom-manage/issues/320
Copy link
Member

Choose a reason for hiding this comment

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

Nitpick: Since this is a private issue, it should use the shorthand. Also, the linked issue is for the inbound link (Anchor→WP.com), but the TODO here refers to the outbound link (WP.com→Anchor). We can maybe create a new follow-up issue.

client/my-sites/customer-home/cards/tasks/podcasting/index.jsx Outdated
actionText={ translate( 'Get started' ) }
// TODO replace with more appropriate URL: https://github.com/Automattic/dotcom-manage/issues/320
actionUrl={ `https://anchor.fm` }
completeOnStart={ false }
Copy link
Member

Choose a reason for hiding this comment

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

One more nitpick: completeOnStart defaults to false so we can remove this altogether.

wp-calypso/client/my-sites/customer-home/cards/tasks/task.jsx

Line 40 in 5da115c

completeOnStart = false,

@blackjackkent
Copy link
Contributor

I went ahead and added updates for the small issues mentioned in Miguel's comments above. :) This looks good to me too though.

@krymson24
Copy link
Contributor Author

I went ahead and added updates for the small issues mentioned in Miguel's comments above. :) This looks good to me too though.

Thanks @blackjackkent !

@krymson24 krymson24 merged commit 3471fc3 into master Nov 19, 2020
@krymson24 krymson24 deleted the anchorfm-card branch November 19, 2020 03:14
@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 Nov 19, 2020
@krymson24 krymson24 changed the title Home: Podcasting Card AnchorFM: Podcasting Card Nov 19, 2020
@a8ci18n
Copy link

a8ci18n commented Nov 19, 2020

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

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

@ghost
Copy link

ghost commented Nov 24, 2020

The Anchor link opening in the same window feels a bit unexpected. Can we change that CTA copy to make it a little clearer this is taking you out of WordPress.com? Maybe something like "Create an Anchor account"

Home

@krymson24
Copy link
Contributor Author

The Anchor link opening in the same window feels a bit unexpected. Can we change that CTA copy to make it a little clearer this is taking you out of WordPress.com? Maybe something like "Create an Anchor account"

Home

@sfougnier PR here: #47726
Issue here: 339-gh-dotcom-manage

@a8ci18n
Copy link

a8ci18n commented Nov 27, 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
Reviewers

@mmtr mmtr mmtr approved these changes

Assignees

@krymson24 krymson24

Labels
Podcasting
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@krymson24 @matticbot @sixhours @blackjackkent @a8ci18n @mmtr