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

Unsupported Block #106

Merged
merged 25 commits into from
Aug 23, 2018
Merged

Unsupported Block #106

merged 25 commits into from
Aug 23, 2018

Conversation

diegoreymendez
Copy link
Contributor

@diegoreymendez diegoreymendez commented Aug 13, 2018

Description:

This PR implements the unsupported block for blocks we don't offer support for.

Fixes #50.

Details:

This PR introduces a new block type to display all the blocks we don't support. Adding support for a block happens through regular registration of the block type in Gutenberg.

Next steps: introduce this new block type as the native version of the freeform block.

Known issues / limitations:

After the changes submitted by @koke in
#127, I could no longer retrieve the original block name to display in visual mode.

This should be fine though, as unsupported blocks could be changing once this is merged: WordPress/gutenberg#8274

Testing:

  • Load the standard post content.
  • Switch to HTML mode and make sure the block contents are properly maintained.

@etoledom
Copy link
Contributor

etoledom commented Aug 13, 2018

After giving it a try, I noticed that it's breaking because getBlockType() fails if it's called with a block name that is not registered and returns undefined. Then it tries to access attributes from that undefined BlockType.

So it's failing when it tries to access blockType.attributes, not block.attributes

At this line it return undefined: serializer.js:259

At this line it try to access attributes:serializer.js:139

I'm not sure what is the best way to fixing this. A try catch avoid the crash but it feels like a regression on behavior:

try {
    return serialize( [ block ] ) + '\n\n';
} catch (error) {
    return block.attributes.content + '\n\n';
}

unsupported

@etoledom
Copy link
Contributor

I tested an alternative strategy to show the unsupported block UI, based on the conversation we had in slack. It's described here.

@diegoreymendez
Copy link
Contributor Author

Ready for review.

}

render() {
let blockName = this.props.name.charAt(0).toUpperCase() + this.props.name.slice(1);
Copy link
Member

Choose a reason for hiding this comment

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

Wouldn’t this always be “Gmobile/unsupported” now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeppers. I can just remove it if it feels redundant, although I left it in cos otherwise the block feels a bit empty at this time.

Let me know which way you prefer it.

Copy link
Member

Choose a reason for hiding this comment

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

Since the other line already says unsupported, I think we can remove it for now

@koke
Copy link
Member

koke commented Aug 22, 2018

Other than the title, this is looking great. The tests are failing on travis though

@koke
Copy link
Member

koke commented Aug 23, 2018

I've fixed the conflicts with master and the tests to get this ready for today's demo. It also turned out that with the latest changes, the block manager didn't need any special case handling for the unsupported block, so that was simplified as well when resolving the conflict: 1a2bfc0

Merging when green

@koke koke merged commit a280f6e into master Aug 23, 2018
@koke koke deleted the issue/50-unsupported-render-serialize-back branch August 23, 2018 07:43
hypest pushed a commit that referenced this pull request Jan 3, 2019
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.

3 participants