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

Convert a Classic Block into Multiple Blocks #3179

Merged
merged 1 commit into from Oct 31, 2017
Merged

Conversation

@ellatrix
Copy link
Member

@ellatrix ellatrix commented Oct 26, 2017

Description

See #3149.

This PR adds a button to the right side dropdown for freeform blocks to convert it into separate blocks.

How Has This Been Tested?

  1. Create a Freeform block.
  2. Click the right menu, then 'Convert to blocks'.
  3. The content of the block should now be parsed into normal blocks.

Thoughts:

  • We might want to rename the handler to something more general, like 'rawHandler' or 'convertStringToBlocks', or maybe a separate function that can be used by pasteHandler too.
return (
<IconButton
className="editor-block-settings-menu__control"
onClick={ () => convertToBlocks( block ) }
Copy link
Member Author

@ellatrix ellatrix Oct 26, 2017

Not entirely sure what the best way is here to avoid passing arguments.

Copy link
Member

@aduth aduth Oct 26, 2017

What's the desire to avoid passing the argument? If you mean so that the component doesn't need to be aware of a block prop, the only way would probably be overriding react-redux connect's third argument mergeProps for the dispatch to be aware of the block from state, but... probably more work than is really worthwhile.

Copy link
Contributor

@ephox-mogran ephox-mogran Oct 26, 2017

Can you make the component not know about the block? Don't you have to return null if the block isn't defined (or has an invalid name)?

@ellatrix ellatrix force-pushed the try/unknown-converter branch 2 times, most recently from ba9ad2b to eb371f2 Oct 26, 2017
( dispatch, { uid } ) => ( {
convertToBlocks( block ) {
dispatch( replaceBlocks( [ uid ], rawHandler( {
HTML: block.attributes.content,
Copy link
Member Author

@ellatrix ellatrix Oct 26, 2017

Although we also set it directly in the paste module, we might want to register this as well with the API. See

return createBlock( getUnknownTypeHandlerName(), {
content: node.outerHTML,
} );

Atm we require the attribute to be content implicitly.

Copy link
Member

@aduth aduth Oct 26, 2017

Could we call serializeBlock here instead?

Copy link
Member Author

@ellatrix ellatrix Oct 27, 2017

Good point. I guess we could. :)

@@ -50,6 +51,7 @@ function BlockSettingsMenu( { uids, onSelect } ) {
<div className="editor-block-settings-menu__content">
<BlockInspectorButton onClick={ onClose } />
{ count === 1 && <BlockModeToggle uid={ uids[ 0 ] } onToggle={ onClose } /> }
{ count === 1 && <UnknownConverter uid={ uids[ 0 ] } /> }
Copy link
Contributor

@youknowriad youknowriad Oct 27, 2017

Does it make sense to include this in BlockToolbar (for mobile controls)?

Copy link
Member Author

@ellatrix ellatrix Oct 27, 2017

I'm not entirely sure what you mean.

Copy link
Contributor

@youknowriad youknowriad Oct 27, 2017

The block settings menu is hidden on mobile and these controls are shown in the block toolbar instead here lob/master/editor/block-toolbar/index.js#L143 Thinking we could do the same with this control.

@@ -6,7 +6,7 @@ import * as source from './source';
export { source };
export { createBlock, switchToBlockType } from './factory';
export { default as parse, getSourcedAttributes } from './parser';
export { default as pasteHandler } from './paste';
export { default as rawHandler } from './paste';
Copy link
Contributor

@youknowriad youknowriad Oct 27, 2017

Should we rename the directory as well?

Copy link
Member Author

@ellatrix ellatrix Oct 27, 2017

Ah, of course.

import { getBlock } from '../selectors';
import { replaceBlocks } from '../actions';

export function UnknownConverter( { block, convertToBlocks } ) {
Copy link
Contributor

@mtias mtias Oct 27, 2017

I don't have any bright ideas, but I think we should call this differently :)

Copy link
Contributor

@mtias mtias Oct 27, 2017

Maybe LegacyConverter or ClassicConverter.

Copy link
Member Author

@ellatrix ellatrix Oct 29, 2017

I've named it this way because we're checking getUnknownTypeHandlerName. :)

Copy link
Member Author

@ellatrix ellatrix Oct 29, 2017

I mean, this is an API thing, not a block thing. The component shouldn't depend on what block handles unknown pieces right?

@mtias
Copy link
Contributor

@mtias mtias commented Oct 27, 2017

This is working well for me, nice to see that the pasting strategy is proving very worthwhile. Also, when undo works properly it feels very effortless to convert and switch back if you didn't like the result.

One other thing that might be nice to add is to trigger a success notice with some text like "Converted to 12 blocks" or something like that, which would convey the more significant nature of the operation.

@ellatrix ellatrix force-pushed the try/unknown-converter branch 3 times, most recently from 0c67b4c to 2743b51 Oct 29, 2017
@ellatrix
Copy link
Member Author

@ellatrix ellatrix commented Oct 29, 2017

One other thing that might be nice to add is to trigger a success notice with some text like "Converted to 12 blocks" or something like that, which would convey the more significant nature of the operation.

Where should this notification pop up? Not really sure if it's any use to the user though? I like showing as little as possible, it just works. :)

@ellatrix
Copy link
Member Author

@ellatrix ellatrix commented Oct 29, 2017

We could also select the converted blocks... (Which kind of indicates something just happened with those blocks.)

@jasmussen
Copy link
Contributor

@jasmussen jasmussen commented Oct 30, 2017

I love this so much. ❤️

@mtias mtias added this to the Beta 1.6 milestone Oct 31, 2017
@@ -77,6 +77,15 @@ describe( 'paste', () => {

equal( filtered, '<em>test</em>' );
} );

it( 'should always return blocks', () => {
const blocks = paste( {
Copy link
Member

@gziolo gziolo Oct 31, 2017

Should it be renamed to rawHandler, too?

( state, { uid } ) => ( {
block: getBlock( state, uid ),
} ),
( dispatch, { uid } ) => ( {
Copy link
Member

@gziolo gziolo Oct 31, 2017

You have block already exposed in props. The following should work:

( dispatch, { block, uid } ) => ( {
	convertToBlocks() {
		dispatch( replaceBlocks( uid, rawHandler( {
			HTML: serialize( block ),
			inline: false,
		} ) ) );
	},
} )

Although @aduth might be correct that you would have to use 3rd param mergeProps here.

Yes, 2nd param is ownProps ...

Copy link
Member

@gziolo gziolo Oct 31, 2017

Another funky way to do the same is to wrap this with HOC withBlock :P

const withBlock = component => connect(  ( state, { uid } ) => ( {
	block: getBlock( state, uid ),
} ) )( component );

export default connect(
	null,
	( dispatch, { block, uid } ) => ( {
		convertToBlocks() {
			dispatch( replaceBlocks( uid, rawHandler( {
				HTML: serialize( block ),
				inline: false,
			} ) ) );
		},
	} )
)( withBlock( UnknownConverter ) );

Copy link
Member Author

@ellatrix ellatrix Oct 31, 2017

I'm fine with keeping as is :)

Copy link
Member

@gziolo gziolo Oct 31, 2017

👍

@ellatrix ellatrix force-pushed the try/unknown-converter branch from 2743b51 to eaefd8a Oct 31, 2017
@@ -78,6 +79,7 @@ class BlockToolbar extends Component {
<BlockInspectorButton small />
<BlockModeToggle uid={ block.uid } small />
<BlockDeleteButton uids={ [ block.uid ] } small />
<UnknownConverter uid={ uid } small />
Copy link
Member

@gziolo gziolo Oct 31, 2017

It seems like block.uid should be passed as a prop instead.

Copy link
Member

@gziolo gziolo Oct 31, 2017

Eslint agrees with me:

/Users/gziolo/PhpstormProjects/vagrant-local/www/wordpress-default/public_html/wp-content/plugins/editor/block-toolbar/index.js
82:32 error 'uid' is not defined no-undef

Copy link
Member Author

@ellatrix ellatrix Oct 31, 2017

Whoops

@ellatrix ellatrix force-pushed the try/unknown-converter branch 2 times, most recently from 8c666d6 to 79f6c6a Oct 31, 2017
@codecov
Copy link

@codecov codecov bot commented Oct 31, 2017

Codecov Report

Merging #3179 into master will increase coverage by 0.36%.
The diff coverage is 23.52%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3179      +/-   ##
==========================================
+ Coverage   31.11%   31.47%   +0.36%     
==========================================
  Files         230      231       +1     
  Lines        6448     6544      +96     
  Branches     1150     1181      +31     
==========================================
+ Hits         2006     2060      +54     
- Misses       3726     3750      +24     
- Partials      716      734      +18
Impacted Files Coverage Δ
blocks/api/raw-handling/ms-list-converter.js 97.43% <ø> (ø)
blocks/api/raw-handling/table-normaliser.js 100% <ø> (ø)
blocks/api/raw-handling/comment-remover.js 100% <ø> (ø)
blocks/api/raw-handling/normalise-blocks.js 96.87% <ø> (ø)
blocks/api/raw-handling/create-unwrapper.js 100% <ø> (ø)
blocks/api/raw-handling/utils.js 98.07% <ø> (ø)
blocks/api/raw-handling/image-corrector.js 100% <ø> (ø)
blocks/api/raw-handling/is-inline-content.js 100% <ø> (ø)
blocks/api/raw-handling/list-merger.js 100% <ø> (ø)
blocks/api/raw-handling/formatting-transformer.js 100% <ø> (ø)
... and 13 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update aee3546...fecc4b3. Read the comment docs.

@@ -78,6 +79,7 @@ class BlockToolbar extends Component {
<BlockInspectorButton small />
<BlockModeToggle uid={ block.uid } small />
<BlockDeleteButton uids={ [ block.uid ] } small />
<UnknownConverter uid={ block.uid } small />
Copy link
Member

@gziolo gziolo Oct 31, 2017

Can we put it before Delete button? This is how it looks at the moment:

screen shot 2017-10-31 at 15 30 27

@jasmussen @karmatosed, what would be the best place to put it?

Copy link
Member Author

@ellatrix ellatrix Oct 31, 2017

Sorry, yeah, should be the same as normal drop down. :)

@ellatrix ellatrix force-pushed the try/unknown-converter branch from 79f6c6a to fecc4b3 Oct 31, 2017
@gziolo
Copy link
Member

@gziolo gziolo commented Oct 31, 2017

I tested it with the following HTML code from the post created with the classic editor:

https://pastebin.com/pkCSmk8f

It works perfectly fine with some exceptions: embed urls are converted into paragraphs (2 urls that were into their own lines were combined into one paragraph). I know it's out of scope here, but is there plan to improve that in the future?

@ellatrix
Copy link
Member Author

@ellatrix ellatrix commented Oct 31, 2017

@gziolo Yeah, that's correct, URLs currently go through the text patterns... We should change this in another PR.

gziolo
gziolo approved these changes Oct 31, 2017
Copy link
Member

@gziolo gziolo left a comment

This works perfectly fine using the same API as we offer with paste functionality. Great job integrating this. Thanks for addressing all issues I raised when looking at this code 🙇

I left a comment about my existing post that would benefit from future improvements applied to rawHandler, but it's rather nice to have.

@gziolo
Copy link
Member

@gziolo gziolo commented Oct 31, 2017

I noticed some issues with Travis but I don't think it has anything with the changes introduced here. It would be nice to confirm before merging this PR.

@ellatrix ellatrix merged commit 95d6c39 into master Oct 31, 2017
0 of 2 checks passed
@ellatrix ellatrix deleted the try/unknown-converter branch Oct 31, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

7 participants