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

Transforms: Shortcode: Support isMatch predicate #18459

Merged
merged 4 commits into from
Nov 19, 2019

Conversation

mcsf
Copy link
Contributor

@mcsf mcsf commented Nov 12, 2019

Description

Fixes #10674

How has this been tested?

Expanded unit tests for shortcode conversion.

Types of changes

Enhancement, API.

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.
  • I've updated all React Native files affected by any refactorings/renamings in this PR. .

@mcsf mcsf added [Feature] Block API API that allows to express the block paradigm. [Feature] Extensibility The ability to extend blocks or the editing experience [Feature] Paste [Feature] Shortcodes Related to shortcode functionality [Feature] Block Transforms Block transforms from one block to another labels Nov 12, 2019
@mcsf mcsf requested review from pento, gziolo and ellatrix November 12, 2019 16:55
<p>[my-broccoli id="42"]</p>
<p>[my-broccoli id="1000"]</p>`;

const transformed = segmentHTMLToShortcodeBlock( original );
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 add some integration tests as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ellatrix, a few of questions:

  1. How would they differ from the current ones? It's not like any core blocks are using isMatch, nor that there are multiple core blocks interested in the same shortcode tag.
  2. Looking at blocks-raw-handling.test.js, I notice that there are now three "areas" for testing:
  • individual tests under Blocks raw handling
  • the fixture-based tests under pasteHandler
  • individual tests under rawHandler

Seems like we could revisit this? I personally don't really know where to start adding tests for this.

  1. That readFile helper silently bails if the provided file path doesn't exist, meaning that types in pasteHandler can just silently pass even when there are missing fixtures. Case in point: caption-shortcode no longer runs. Seems like something to fix too, I don't see a reason not to throw an error if a file is missing.

Copy link
Member

Choose a reason for hiding this comment

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

It's not like any core blocks are using isMatch, nor that there are multiple core blocks interested in the same shortcode tag.

Interestingly, this could be changed. An example off the top of my head is that the [video] shortcode in Core explicitly supports YouTube and Vimeo URLs in the src attribute. These shortcodes could be transformed into their respective core-embed block, rather than the core/video block.

Copy link
Member

@ellatrix ellatrix Nov 18, 2019

Choose a reason for hiding this comment

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

  1. How would they differ from the current ones? It's not like any core blocks are using isMatch, nor that there are multiple core blocks interested in the same shortcode tag.

That's true, but we could add a test block?

Seems like we could revisit this? I personally don't really know where to start adding tests for this.

Yeah, we should clean that up :) Ideally, if applicable, tests should be run for both the raw and paste handler.

That readFile helper silently bails if the provided file path doesn't exist, meaning that types in pasteHandler can just silently pass even when there are missing fixtures. Case in point: caption-shortcode no longer runs. Seems like something to fix too, I don't see a reason not to throw an error if a file is missing.

Me neither. Sounds good to throw an error.

@ellatrix
Copy link
Member

Looks great, but would really love some integration tests for an extra layer of confidence. :)

@pento
Copy link
Member

pento commented Nov 12, 2019

Thank you for getting onto this one, @mcsf!

For consistency, is it worth looking at having the shortcode transform use a transform function, too, so it matches all of the other types? It seems a bit weird to have a different pattern just for shortcodes. @ellatrix, I don't suppose you recall any additional conversations around #2874?

@ellatrix
Copy link
Member

@pento What kind of additional conversation? I believe we use isMatch for other transforms too.

@pento
Copy link
Member

pento commented Nov 13, 2019

Talking about the difference between the examples in the docs, all of the transform types provide a transform property, which is a function to create the block, except for the shortcode type, which has the attributes property, instead. I wasn't sure if this had been discussed in Slack or somewhere else, I couldn't find any relevant conversation. 🙂

@ellatrix
Copy link
Member

I don't think so. I'm fine with revising that. The difference is that the other transform functions are provided with block attributes, in this case they would be shortcode attributes.

@mcsf
Copy link
Contributor Author

mcsf commented Nov 15, 2019

Talking about the difference between the examples in the docs, all of the transform types provide a transform property, which is a function to create the block, except for the shortcode type, which has the attributes property, instead. I wasn't sure if this had been discussed in Slack or somewhere else, I couldn't find any relevant conversation.

I know I raised this way way back somewhere in a comment, deep in the GitHub mist.

type: 'shortcode',
tag: 'gallery',
isMatch( { named: { ids } } ) {
return ids.indexOf( 42 ) > -1;
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 your answer to everything? :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

😄

@mcsf mcsf force-pushed the add/shortcodes-isMatch-support branch from 6c020eb to 9a1848b Compare November 19, 2019 09:13
@mcsf mcsf merged commit f63b197 into master Nov 19, 2019
@mcsf mcsf deleted the add/shortcodes-isMatch-support branch November 19, 2019 11:04
@pento pento added this to the Gutenberg 7.0 milestone Nov 19, 2019
@jorgefilipecosta jorgefilipecosta added the Needs Dev Note Requires a developer note for a major WordPress release cycle label Feb 4, 2020
@jorgefilipecosta jorgefilipecosta mentioned this pull request Feb 12, 2020
23 tasks
@youknowriad youknowriad removed the Needs Dev Note Requires a developer note for a major WordPress release cycle label Apr 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Block API API that allows to express the block paradigm. [Feature] Block Transforms Block transforms from one block to another [Feature] Extensibility The ability to extend blocks or the editing experience [Feature] Paste [Feature] Shortcodes Related to shortcode functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Transforms: "isMatch" does not work for shortcode transformation
5 participants