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.

Sign up for GitHub

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

Update/refactor converters #7885

Merged
merged 6 commits into from
Jul 12, 2018
Merged

Update/refactor converters #7885

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
e43de24
Move convertToBlock function to withDispatch
oandregal Jul 11, 2018
c0b7cb5
Create the converters out of an unique Converter UI component
oandregal Jul 11, 2018
21e0327
Consolidate into converters.js
oandregal Jul 11, 2018
8c8d9fe
Separate components in their own file
oandregal Jul 11, 2018
cb4e630
Rename files
oandregal Jul 12, 2018
0327937
Rename
oandregal Jul 12, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 24 additions & 0 deletions editor/components/block-settings-menu/convertToBlocks.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
/**
* WordPress dependencies
*/
import { __ } from '@wordpress/i18n';
import { IconButton } from '@wordpress/components';

export default function ConvertToBlocks( { shouldRender, onClick, small, role } ) {
if ( ! shouldRender ) {
return null;
}

const label = __( 'Convert to Blocks' );
return (
<IconButton
className="editor-block-settings-menu__control"
onClick={ onClick }
icon="screenoptions"
label={ small ? label : undefined }
role={ role }
>
{ ! small && label }
</IconButton>
);
}
33 changes: 33 additions & 0 deletions editor/components/block-settings-menu/convertToBlocksFromHTML.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
/**
* WordPress dependencies
*/
import { rawHandler, getBlockContent } from '@wordpress/blocks';
import { compose } from '@wordpress/element';
import { withSelect, withDispatch } from '@wordpress/data';

/**
* Internal dependencies
*/
import ConvertToBlocks from './convertToBlocks';

export default compose(
withSelect( ( select, { uid } ) => {
const { getBlock, canUserUseUnfilteredHTML } = select( 'core/editor' );
const block = getBlock( uid );
return {
block,
canUserUseUnfilteredHTML: canUserUseUnfilteredHTML(),
shouldRender: ( block && block.name === 'core/html' ),
};
} ),
withDispatch( ( dispatch, { block, canUserUseUnfilteredHTML } ) => ( {
onClick: () => dispatch( 'core/editor' ).replaceBlocks(
block.uid,
rawHandler( {
HTML: getBlockContent( block ),
mode: 'BLOCKS',
canUserUseUnfilteredHTML,
} ),
),
} ) ),
)( ConvertToBlocks );
33 changes: 33 additions & 0 deletions editor/components/block-settings-menu/convertToBlocksFromUnknown.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
/**
* WordPress dependencies
*/
import { getUnknownTypeHandlerName, rawHandler, serialize } from '@wordpress/blocks';
import { compose } from '@wordpress/element';
import { withSelect, withDispatch } from '@wordpress/data';

/**
* Internal dependencies
*/
import ConvertToBlocks from './convertToBlocks';

export default compose(
withSelect( ( select, { uid } ) => {
const { canUserUseUnfilteredHTML, getBlock } = select( 'core/editor' );
const block = getBlock( uid );
return {
block,
canUserUseUnfilteredHTML: canUserUseUnfilteredHTML(),
shouldRender: ( block && block.name === getUnknownTypeHandlerName() ),
};
} ),
withDispatch( ( dispatch, { block, canUserUseUnfilteredHTML } ) => ( {
onClick: () => dispatch( 'core/editor' ).replaceBlocks(
block.uid,
rawHandler( {
HTML: serialize( block ),
mode: 'BLOCKS',
canUserUseUnfilteredHTML,
} )
),
} ) ),
)( ConvertToBlocks );
50 changes: 0 additions & 50 deletions editor/components/block-settings-menu/html-converter.js
View file
Open in desktop

This file was deleted.

8 changes: 4 additions & 4 deletions editor/components/block-settings-menu/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

good catch! Will fix when we agree on the names.

import ConvertToBlocksFromUnknownButton from './convertToBlocksFromUnknown';
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member

@gziolo gziolo Jul 12, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

How about BlockConverterFromHTMLButton and BlockConverterFromUnknownButton? That way we follow the current naming schema (other files either start by block or shared-block).

Copy link
Member Author

Choose a reason for hiding this comment

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

cb4e630

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

import _BlockSettingsMenuFirstItem from './block-settings-menu-first-item';

export class BlockSettingsMenu extends Component {
Expand Down Expand Up @@ -95,8 +95,8 @@ export class BlockSettingsMenu extends Component {
<NavigableMenu className="editor-block-settings-menu__content">
<_BlockSettingsMenuFirstItem.Slot fillProps={ { onClose } } />
{ count === 1 && <BlockModeToggle uid={ firstBlockUID } onToggle={ onClose } role="menuitem" /> }
{ count === 1 && <UnknownConverter uid={ firstBlockUID } role="menuitem" /> }
{ count === 1 && <HTMLConverter uid={ firstBlockUID } role="menuitem" /> }
{ count === 1 && <ConvertToBlocksFromUnknownButton uid={ firstBlockUID } role="menuitem" /> }
{ count === 1 && <ConvertToBlocksFromHTMLButton uid={ firstBlockUID } role="menuitem" /> }
<BlockDuplicateButton uids={ uids } rootUID={ rootUID } role="menuitem" />
{ count === 1 && <SharedBlockConvertButton uid={ firstBlockUID } onToggle={ onClose } itemsRole="menuitem" /> }
<div className="editor-block-settings-menu__separator" />
Expand Down
50 changes: 0 additions & 50 deletions editor/components/block-settings-menu/unknown-converter.js
View file
Open in desktop

This file was deleted.