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

Rename the Speaker block to Speaker Deck #9727

Merged
merged 2 commits into from Sep 12, 2018

Conversation

Projects
None yet
5 participants
@pento
Member

pento commented Sep 10, 2018

For some reason the Speaker Deck block is called "Speaker", this PR replaces it with a new "Speaker Deck" block, and transforms the old blocks into new blocks.

Fixes #7850.

How has this been tested?

  • Create a post with a Speaker block
  • Switch to this PR
  • Observe the block being silently upgraded to the Speaker Deck block.

Also, check that there's no way to insert an old Speaker 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.
Deprecate the core-embed/speaker block
Add a new core-embed/speaker-deck block to replace it, and a transform to turn the old blocks into the new blocks.

@pento pento requested a review from WordPress/gutenberg-core Sep 10, 2018

@tofumatt

Makes sense 👍

@tofumatt tofumatt added this to the 3.9 milestone Sep 10, 2018

@pento

This comment has been minimized.

Show comment
Hide comment
@pento

pento Sep 10, 2018

Member

Thinking about this some more, I didn't like having the transform on the old block, it kind of made sense from a "here's what this block turns into" perspective, but it's not how we tell people to use transform. 🙂

I removed the URL pattern from the old block and added a comment to point out the deprecation.

Member

pento commented Sep 10, 2018

Thinking about this some more, I didn't like having the transform on the old block, it kind of made sense from a "here's what this block turns into" perspective, but it's not how we tell people to use transform. 🙂

I removed the URL pattern from the old block and added a comment to point out the deprecation.

@aduth

This comment has been minimized.

Show comment
Hide comment
@aduth

aduth Sep 10, 2018

Member

High-level question: My first thought was implementing as a block deprecation, but we don't support the idea of deprecating from one block type to another block type. Is that something we should support? I could, for example, imagine the migrate function being overloaded to support returning a block instance (optionally of another type) rather than an object of attributes.

Member

aduth commented Sep 10, 2018

High-level question: My first thought was implementing as a block deprecation, but we don't support the idea of deprecating from one block type to another block type. Is that something we should support? I could, for example, imagine the migrate function being overloaded to support returning a block instance (optionally of another type) rather than an object of attributes.

@youknowriad

This comment has been minimized.

Show comment
Hide comment
@youknowriad

youknowriad Sep 10, 2018

Contributor

High-level question: My first thought was implementing as a block deprecation, but we don't support the idea of deprecating from one block type to another block type.

This came up a few times and we do have some "hidden" blocks that can be removed if we implement this: (subheading, text columns). We'd need to make sure the transformation only triggers if the original block is not registered to avoid "stealing blocks :P"

Contributor

youknowriad commented Sep 10, 2018

High-level question: My first thought was implementing as a block deprecation, but we don't support the idea of deprecating from one block type to another block type.

This came up a few times and we do have some "hidden" blocks that can be removed if we implement this: (subheading, text columns). We'd need to make sure the transformation only triggers if the original block is not registered to avoid "stealing blocks :P"

@pento

This comment has been minimized.

Show comment
Hide comment
@pento

pento Sep 10, 2018

Member

I had a similar first thought, but ran into the problem of that not being possible. 🙂

Having a proper way to deprecate a block type would fix several of the issues in this PR:

  • Having to keep the old block type registered, but hidden.
  • The entire transform section feeling not quite right.
  • Having to put a comment on the old block, explaining the deprecation.
Member

pento commented Sep 10, 2018

I had a similar first thought, but ran into the problem of that not being possible. 🙂

Having a proper way to deprecate a block type would fix several of the issues in this PR:

  • Having to keep the old block type registered, but hidden.
  • The entire transform section feeling not quite right.
  • Having to put a comment on the old block, explaining the deprecation.
@pento

This comment has been minimized.

Show comment
Hide comment
@pento

pento Sep 12, 2018

Member

Merging this one, we can think more about what a good way to handle renaming blocks should look like.

Member

pento commented Sep 12, 2018

Merging this one, we can think more about what a good way to handle renaming blocks should look like.

@pento pento merged commit 4d8981c into master Sep 12, 2018

2 checks passed

codecov/project 49.02% (-0.01%) compared to 410f78a
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@pento pento deleted the fix/7850-speaker-deck branch Sep 12, 2018

@chrisvanpatten

This comment has been minimized.

Show comment
Hide comment
@chrisvanpatten

chrisvanpatten Sep 14, 2018

Contributor

Just came across this PR while trying to figure out if it were possible to rename a block. I'd definitely love that capability and I expect other block devs would too.

Contributor

chrisvanpatten commented Sep 14, 2018

Just came across this PR while trying to figure out if it were possible to rename a block. I'd definitely love that capability and I expect other block devs would too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment