Update/refactor converters #7885
Conversation
There was a problem hiding this comment.
We try to keep one component per file whenever it makes sense. In 99% of cases it does. We can refactor this PR to follow the same principle.
What do you think about having the following structure instead:
-- block-setting-menu
-- converter.js
-- html-converter.js
-- unknown-converter.js
We can also use ifCondition HOC to make those implementation components really simple:
// unknown-converter.js
/**
* WordPress dependencies
*/
import { ifCondition } from '@wordpress/components';
/**
* Internal dependencies
*/
import Converter from './converter';
export ifCondition(
( { block } ) => block && block.name === getUnknownTypeHandlerName()
)( Converter );| /** | ||
| * WordPress dependencies | ||
| */ | ||
| import { __ } from '@wordpress/i18n'; |
There was a problem hiding this comment.
This is a nice example of how much you can achieve using only packages offered by Gutenberg 💯
There was a problem hiding this comment.
Didn't realize that! :gutenberg-power:
| shouldRender: ( block && block.name === getUnknownTypeHandlerName() ), | ||
| }; | ||
| } ), | ||
| withDispatch( ( dispatch, { block, canUserUseUnfilteredHTML } ) => ( { |
There was a problem hiding this comment.
Please note that withSelect and withDispatch differ only in one line:
shouldRender: ( block && block.name === getUnknownTypeHandlerName() ),
// vs
shouldRender: ( block && block.name === 'core/html' ),
| onClick: () => dispatch( 'core/editor' ).replaceBlocks( | ||
| block.uid, | ||
| rawHandler( { | ||
| HTML: serialize( block ), |
There was a problem hiding this comment.
I now see:
serialize vs getBlockContent
There was a problem hiding this comment.
it might be more difficult to refactor the way I proposed, having two withDispatch implementation is also the way to go.
|
@gziolo components live now in their own file, is there anything else we can do to improve this or would this be ready to merge? |
| @@ -21,8 +21,8 @@ import BlockDuplicateButton from './block-duplicate-button'; | |||
| import BlockRemoveButton from './block-remove-button'; | |||
| import SharedBlockConvertButton from './shared-block-convert-button'; | |||
| import SharedBlockDeleteButton from './shared-block-delete-button'; | |||
| import UnknownConverter from './unknown-converter'; | |||
| import HTMLConverter from './html-converter'; | |||
| import ConvertToBlocksFromHTMLButton from './convertToBlocksFromHTML'; | |||
There was a problem hiding this comment.
I noticed that file names use camel case notation, which differs from all other files as you can see above. Other file names also end with -button which aligns with the component name.
There was a problem hiding this comment.
good catch! Will fix when we agree on the names.
| import UnknownConverter from './unknown-converter'; | ||
| import HTMLConverter from './html-converter'; | ||
| import ConvertToBlocksFromHTMLButton from './convertToBlocksFromHTML'; | ||
| import ConvertToBlocksFromUnknownButton from './convertToBlocksFromUnknown'; |
There was a problem hiding this comment.
ConvertToBlocksFromUnknownButton - it would be nice to find a different name. At the moment the colocation of unknown and button is a bit unfortunate :(
HtmlToBlocksConvertButton - how about this?
There was a problem hiding this comment.
According to our updated coding guidelines:
https://github.com/WordPress/gutenberg/blob/4263e556fc889206fee5b22d54a1cd910f4e586b/docs/reference/coding-guidelines.md#abbreviations-and-acronyms
It would be: HTMLToBlocksConvertButton and UnknownToBlocksConvertButton.
There was a problem hiding this comment.
I'd like to find a name that collocates the three components together, like SharedBlockConvertButton and SharedBlockConvertButton convey that they act on shared blocks.
Not sure how to improve the current ones 🤔
There was a problem hiding this comment.
How about BlockConverterFromHTMLButton and BlockConverterFromUnknownButton? That way we follow the current naming schema (other files either start by block or shared-block).
There was a problem hiding this comment.
My point was that it reads as:
from HTML button and from unknown button
vs
blocks convert button
Your call really. It's not a blocker for me, just my observation.
There was a problem hiding this comment.
I appreciate the time you've invested to share that observation and to help me figure out this!
In 0327937 I decided for BlockUnknownConvertButton and BlockHTMLConvertButton which addresses the concerns and is more in line with our current naming schema. We can iterate later if necessary.
|
😃 |
Follows up #7667