-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Create the proper shortcode on paste #21864
Conversation
Size Change: -28.5 kB (3%) Total Size: 816 kB
ℹ️ View Unchanged
|
const pieces = shortcodeConverter( HTML ); | ||
|
||
// The call to shortcodeConverter will always return more than one element | ||
// if shortcodes are matched. The reason is when shortcodes are matched | ||
// empty HTML strings are included. | ||
const hasShortcodes = pieces.length > 1; | ||
|
||
if ( mode === 'INLINE' && ! hasShortcodes ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems to me that this should be fine because shortcodes are converted to blocks, which can't be inline. cc @ellatrix
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've only checked the default shortcodes, though. Perhaps there is some 3rd party shortcode that could be converted into inline formats. However, I don't see that's possible to do right now with the transformations API.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I think it's fine if all the tests pass.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I disagree, I think it's fairly important to respect any third-party inline shortcodes. In my experience, the use cases are many and contemplate anything from inline formatting to marking positions in the document. (The question of whether shortcodes remain the right tool is not ours to ask here!)
It seems to me that this should be fine because shortcodes are converted to blocks, which can't be inline
Because even shortcodes that don't have a known associated block type will be picked up by the catch-all Shortcode block, we would be suddenly forcing all shortcodes to be block-like. One could let the editor differentiate between shortcodes with a known block type (which can never be inline) and unknown shortcodes (which may or not be inline), but I'd honestly stay away from implementing that. It would introduce a lot of complexity for little benefit.
Instead, it would be more interesting if the block editor were smart enough to decide when a pasted shortcode is meant to be inline or not. If it is, the editor should abstain from trying to make it a block. Concretely, these are some behaviours to consider:
- Start a new line (i.e. a blank Paragraph) and paste a shortcode → Add a block for that shortcode.
- Place the caret in the middle of some text and paste a shortcode → Insert the shortcode text verbatim, no new blocks.
- Place the caret at the end of a paragraph of text and paste a shortcode → …? (up to us to decide)
cc @hrkhal you may be interested in testing this one. |
Maybe it's good to have @mcsf's opinion as well, since you did some work on this. :) |
I can't recall: were there specific Puppeteer issues keeping from having e2e tests for pasting? It could be nice to add some. |
const pieces = shortcodeConverter( HTML ); | ||
|
||
// The call to shortcodeConverter will always return more than one element | ||
// if shortcodes are matched. The reason is when shortcodes are matched | ||
// empty HTML strings are included. | ||
const hasShortcodes = pieces.length > 1; | ||
|
||
if ( mode === 'INLINE' && ! hasShortcodes ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I disagree, I think it's fairly important to respect any third-party inline shortcodes. In my experience, the use cases are many and contemplate anything from inline formatting to marking positions in the document. (The question of whether shortcodes remain the right tool is not ours to ask here!)
It seems to me that this should be fine because shortcodes are converted to blocks, which can't be inline
Because even shortcodes that don't have a known associated block type will be picked up by the catch-all Shortcode block, we would be suddenly forcing all shortcodes to be block-like. One could let the editor differentiate between shortcodes with a known block type (which can never be inline) and unknown shortcodes (which may or not be inline), but I'd honestly stay away from implementing that. It would introduce a lot of complexity for little benefit.
Instead, it would be more interesting if the block editor were smart enough to decide when a pasted shortcode is meant to be inline or not. If it is, the editor should abstain from trying to make it a block. Concretely, these are some behaviours to consider:
- Start a new line (i.e. a blank Paragraph) and paste a shortcode → Add a block for that shortcode.
- Place the caret in the middle of some text and paste a shortcode → Insert the shortcode text verbatim, no new blocks.
- Place the caret at the end of a paragraph of text and paste a shortcode → …? (up to us to decide)
This reverts commit e75e1c3.
@mcsf I've pushed a different approach that implements your suggestion. I think it's simpler if we left the use case 3 as it is at the moment, otherwise, we may be preventing the use of certain inline shortcodes that may behave like the end of block markers (by virtue of converting them blocks). |
Yeah, this makes sense. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good now, thanks for fixing!
@@ -39,6 +39,7 @@ import { | |||
} from '@wordpress/rich-text'; | |||
import deprecated from '@wordpress/deprecated'; | |||
import { isURL } from '@wordpress/url'; | |||
import { regexp } from '@wordpress/shortcode'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't see this code at the time of approval. I feel like this belongs in the paste handler together with the rest of the short code logic?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think code may have changed since your approval and Miguel's. I didn't think of pinging you for your re-approval after Miguel's approved changes, sorry.
However, I don't know how we can move this logic to the paste handler. The trick here is to switch to BLOCKS
mode if 1) we're in an empty line and 2) the pasted text is a shortcode. This logic requires knowledge of the rich text value and the pasted content. My thinking was that having this on the rich-text component would make sense as we already do the same for URLs. How does this look? Happy to refactor if another approach is preferred.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also the dependencies are missing in the package file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also the dependencies are missing in the package file
Aside: I thought we had an ESLint rule which verified that any referenced dependencies should be listed in package.json
🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ouch 😞 Here's the fix for the missing dependency #22086
Not sure I've done this before so I may have missed something.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very strange indeed. It fails locally!
⇒ npm run lint-js
> gutenberg@8.0.0 lint-js /Users/andrew/Documents/Code/gutenberg
> wp-scripts lint-js
/Users/andrew/Documents/Code/gutenberg/packages/block-editor/src/components/rich-text/index.js
42:1 error '@wordpress/shortcode' should be listed in the project's dependencies. Run 'npm i -S @wordpress/shortcode' to add it import/no-extraneous-dependencies
✖ 1 problem (1 error, 0 warnings)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Travis job for this PR didn't report anything neither for all subsequent PRs. Just looked at the logs and there aren't any messages about the failure either (it could be that the exit code was wrong for some reason but the message was there).
I can confirm that npm run lint-js
and npm run lint
do fail locally on master
prior to the merge of #22086 Puzzling.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aside: I thought we had an ESLint rule which verified that any referenced dependencies should be listed in
package.json
🤔
Oh, that's unfortunate, I let my guard down too. 😞
I didn't see this code at the time of approval. I feel like this belongs in the paste handler together with the rest of the short code logic?
Perhaps, but there are a lot of symmetries with what we do with pasted URLs and embeds. Whatever we do, we should do for both shortcodes and embeds. Perhaps that means letting pasteHandler
choose whether to force mode: 'BLOCKS'
when initial mode
is 'AUTO'
; perhaps that means exposing a utility like inferMode
or shouldForceBlocksMode
from @wordpress/blocks
that RichText can use.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very strange indeed. It fails locally!
After some more debugging, I think I've figured it out.
Fix (and debugging details) at: #22088
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I don't mind the code, it's something we can always easily change. I was thinking we could also pass empty state to the raw handler. I just thought it might be nice to keep shortcode dependencies in the raw handler since it already depends on it and RichText doesn't. I just noticed the code when I saw the package error.
Fixes #21103
How to test
[gallery ids="13,12,14"]
(change these ids for ids of elements in your media library).