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

Replace Polldaddy embed block with Crowdsignal #12854

Merged
merged 5 commits into from Jan 29, 2019

Conversation

Projects
None yet
5 participants
@ice9js
Copy link
Contributor

ice9js commented Dec 13, 2018

Description

Given the recent rebranding of Polldaddy to Crowdsignal, this change replaces the existing Polldaddy embed type with Crowdsignal and updates the pattern to match Crowdsignal's updated URLs for polls and surveys.

Solves #11517.

How has this been tested?

Before applying my changes, add a post with the current Polldaddy embed block.
After the changes have been applied the block should be automatically upgraded to Crowdsignal.

There should now also be a new Crowdsignal embed block available from the menu.

All current poll and survey links from Crowdsignal should trigger the Crowdsignal embed block when pasted directly into the editor, for example:

I have tested this locally.

Screenshots

screen shot 2018-12-13 at 21 38 58

Types of changes

New feature: Added a Crowdsignal embed block

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.

@gziolo gziolo requested a review from notnownikki Dec 14, 2018

@@ -227,13 +245,14 @@ export const others = [
patterns: [ /^http:\/\/g?i*\.photobucket\.com\/.+/i ],
},
{
// Deprecated in favour of the core-embed/crowdsignal block

This comment has been minimized.

@gziolo

gziolo Dec 14, 2018

Member

I'm not sure if we can remove the pattern because of backward compatibility. However we can hide the deprecated version from the inserter. We did that for subheading block and the old version of columns block.

This comment has been minimized.

@notnownikki

notnownikki Dec 14, 2018

Member

Removing the patterns here is fine, they're only used to resolve URLs into the correct block, so existing blocks are loaded, and converted to the Crowdsignal block without a problem.

I'm not sure we should hide the old block either. It's useful for people who still have the polldaddy name in their minds, and if they use the polldaddy block, it'll convert to a crowdsignal block automatically.

This comment has been minimized.

@gziolo

gziolo Dec 14, 2018

Member

You can resolve it by adding a keyword to the new block.

This comment has been minimized.

@notnownikki

notnownikki Dec 14, 2018

Member

That doesn't help people scrolling through the list instead of typing to search though.

This comment has been minimized.

@gziolo

gziolo Dec 14, 2018

Member

Yes, true. That’s the broader issue with rebranding 😅 I will leave it up to you. We should have some consistent strategy for depreciations.

This comment has been minimized.

@pento

pento Jan 8, 2019

Member

I think it's fine to remove it from the inserter, check the deprecation of core-embed/speaker in favour of core-embed/speaker-deck.

For people still looking for "Polldaddy", perhaps add "formerly Polldaddy" to the description of the new Crowdsignal block, and maybe to the name, too.

This comment has been minimized.

@gziolo

gziolo Jan 29, 2019

Member

ac09b7c - removed from the inserter similar to core-embed/speaker as suggested

This comment has been minimized.

@gziolo

gziolo Jan 29, 2019

Member

b9584b6 - added formerly Polldaddy to the description

This comment has been minimized.

@gziolo

gziolo Jan 29, 2019

Member

d0b2233 - added polldaddy keyword for better discoverability of the new block

@notnownikki
Copy link
Member

notnownikki left a comment

Looks good! I think we should revisit it in a while to remove the polldaddy block from the inserter, once enough time has passed that people are used to the Crowdsignal rebrand.

🚢

@aduth aduth referenced this pull request Jan 4, 2019

Closed

Change PollDaddy embed to Crowdsignal #13180

5 of 5 tasks complete
@pento

This comment has been minimized.

Copy link
Member

pento commented Jan 8, 2019

Note that the renaming has been done in Core already, so this PR needs to be finished/merged in time for WordPress 5.1.

https://core.trac.wordpress.org/ticket/45036

@ice9js ice9js force-pushed the ice9js:add/crowdsignal-embed branch from fa26800 to 60cdd07 Jan 9, 2019

@ice9js

This comment has been minimized.

Copy link
Contributor Author

ice9js commented Jan 9, 2019

I rebased the branch and resolved conflicts.

@gziolo gziolo added this to the 5.0 (Gutenberg) milestone Jan 25, 2019

@gziolo

This comment has been minimized.

Copy link
Member

gziolo commented Jan 25, 2019

@youknowriad and @pento - it looks like it wasn’t included in WordPress 5.1 beta, is it something that needs to be fixed?

@pento

This comment has been minimized.

Copy link
Member

pento commented Jan 29, 2019

@gziolo: Thanks for finding this one. Yes, it would be best if this were included in WordPress 5.1, so everything matches. I've moved #11517 to the WP 5.1 milestone.

@youknowriad

This comment has been minimized.

Copy link
Contributor

youknowriad commented Jan 29, 2019

Sanity check, is this ready to land? I'm planning to update Core today, so this needs to lands ASAP :)

gziolo added some commits Jan 29, 2019

@gziolo

gziolo approved these changes Jan 29, 2019

Copy link
Member

gziolo left a comment

It tests well, I can see how old embed blocks are transformed from Polldaddy to Crowdsignal when the editor is loaded. It is possible to type polldaddy and you will get new Crowdsignal block, the old one no longer appears in search results.

@gziolo

This comment has been minimized.

Copy link
Member

gziolo commented Jan 29, 2019

@ice9js congratulations on your first contribution to Gutenberg 🎉 💯

I will merge this PR as soon as Travis CI will turn green.

@gziolo gziolo merged commit b35774d into WordPress:master Jan 29, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

youknowriad added a commit that referenced this pull request Jan 29, 2019

Replace Polldaddy embed block with Crowdsignal (#12854)
* Replace Polldaddy embed block with Crowdsignal

* Remove Polldaddy from the inserter

* Update core-embeds.js

* Update core-embeds.js

* Update core-embeds.js
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment