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

Format API #10209

Merged
merged 28 commits into from Oct 26, 2018
Merged

Format API #10209

merged 28 commits into from Oct 26, 2018

Conversation

@ellatrix
Copy link
Member

@ellatrix ellatrix commented Sep 27, 2018

Description

Fixes: #10068, #4658.

This PR does a couple of things:

  • It introduces a way to register formats through the rich-text package, which will also be used by Gutenberg itself for the "core" formats in the format-library package.
  • It will include the link format, so it can include all its components (no need to see it as an exception any more).
  • It will include the image format, so there's no longer a need for the token API. The format API will absorb the token API. We can deprecate the token API and keep it around for a while longer.
  • It will be possible to register shortcuts, so we can remove the separate registration of shortcuts (which is currently done through TinyMCE).
  • Adjusts the internal value to use the format name and attributes instead of the element name and attributes.

For more information, see #10068.

How has this been tested?

Screenshots

Types of changes

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
@ellatrix ellatrix force-pushed the try/formatting-api-2 branch from 602ea12 to e121387 Sep 27, 2018
@ellatrix ellatrix self-assigned this Sep 27, 2018
@ellatrix ellatrix force-pushed the try/formatting-api-2 branch from e121387 to b8ad7bd Oct 2, 2018
@ellatrix
Copy link
Member Author

@ellatrix ellatrix commented Oct 2, 2018

Rebased after #7890 merge.

@gziolo gziolo added this to the 4.1 milestone Oct 10, 2018
@gziolo gziolo mentioned this pull request Oct 10, 2018
4 tasks
@ellatrix ellatrix force-pushed the try/formatting-api-2 branch 3 times, most recently from fa41550 to 7e0c850 Oct 13, 2018
@ellatrix ellatrix changed the title WIP: Formatting API Format API Oct 13, 2018
* @return {boolean} True if the parameter is a valid icon and false otherwise.
*/

export function isValidIcon( icon ) {
Copy link
Member Author

@ellatrix ellatrix Oct 15, 2018

Ideally this needs to be shared with the blocks package,

Copy link
Member

@gziolo gziolo Oct 15, 2018

Can we use the one from blocks both isValidIcon and normalizeIconObject? Inline block is one of the special formatter options, so this would make sense. Any blockers?

Copy link
Member Author

@ellatrix ellatrix Oct 15, 2018

I'm not sure I'm understanding this. Should I import them from the blocks package?

Copy link
Member

@gziolo gziolo Oct 15, 2018

I assume, yes we can. Is there any reason to not do it? The inline image is very similar to a block, so they should share the same logic.

@gziolo gziolo removed this from the 4.1 milestone Oct 15, 2018
@gziolo gziolo added this to the 4.2 milestone Oct 15, 2018
@gziolo
Copy link
Member

@gziolo gziolo commented Oct 15, 2018

I'm moving it to 4.2 as we decided that 4.1 should be UI freeze focused. 4.2 is planned to be focused on API freeze and this PR fits perfectly to that description :)

@gziolo gziolo self-requested a review Oct 15, 2018
Copy link
Member

@gziolo gziolo left a comment

I did my first pass of review. This PR is very big. I will play with it in the browser later today.

@@ -71,28 +70,11 @@ function Toolbar( { controls = [], children, className, isCollapsed, icon, label
<ToolbarContainer className={ classnames( 'components-toolbar', className ) }>
{ flatMap( controlSets, ( controlSet, indexOfSet ) => (
controlSet.map( ( control, indexOfControl ) => (
<ToolbarButtonContainer
<ToolbarButton
key={ [ indexOfSet, indexOfControl ].join() }
className={ indexOfSet > 0 && indexOfControl === 0 ? 'has-left-divider' : null }
Copy link
Member

@gziolo gziolo Oct 15, 2018

It looks like this should be renamed to containerClassName.

Copy link
Member Author

@ellatrix ellatrix Oct 15, 2018

I'm not following :)

Copy link
Member

@gziolo gziolo Oct 15, 2018

With the refactoring applied className will be passed to IconButton button instead of ToolbarButtonContainer:

screen shot 2018-10-15 at 17 24 09

@@ -59,6 +60,7 @@ export function initializeEditor( id, postType, postId, settings, overridePost )
const reboot = reinitializeEditor.bind( null, postType, postId, target, settings, overridePost );

registerCoreBlocks();
registerCoreFormats();
Copy link
Member

@gziolo gziolo Oct 15, 2018

I'm wondering if we should register core formats right away instead. In effect, don't offer this method at all.

Copy link
Member

@aduth aduth Oct 16, 2018

I'm wondering if we should register core formats right away instead. In effect, don't offer this method at all.

💯 registerCoreBlock is a necessary hack, and certainly shouldn't serve as an ideal precedent.

@@ -123,7 +123,7 @@ export class BlockSwitcher extends Component {
title={ __( 'Transform To:' ) }
initialOpen
>
<BlockTypesList
<InserterList
Copy link
Member

@gziolo gziolo Oct 15, 2018

I'm not convinced this name change is expected. This is list of all possible transforms, not necessarily a list of blocks to insert.

screen shot 2018-10-15 at 14 46 30

Copy link
Member

@gziolo gziolo Oct 15, 2018

I also feel like it would be better to keep this change out of the scope of this PR to make it smaller. It isn't part of the public API.

packages/format-library/src/image/index.js Outdated Show resolved Hide resolved
packages/editor/src/components/inserter-list/index.js Outdated Show resolved Hide resolved
* @return {boolean} True if the parameter is a valid icon and false otherwise.
*/

export function isValidIcon( icon ) {
Copy link
Member

@gziolo gziolo Oct 15, 2018

Can we use the one from blocks both isValidIcon and normalizeIconObject? Inline block is one of the special formatter options, so this would make sense. Any blockers?

import * as selectors from './selectors';
import * as actions from './actions';

registerStore( 'core/formats', { reducer, selectors, actions } );
Copy link
Member

@gziolo gziolo Oct 15, 2018

It should be core/rich-text namespace.

Copy link
Member Author

@ellatrix ellatrix Oct 15, 2018

Ok

expect( formatTypes( undefined, {} ) ).toEqual( {} );
} );

it( 'should add add a new block type', () => {
Copy link
Member

@gziolo gziolo Oct 15, 2018

Copy and paste -> block

} );
} );

it( 'should remove block types', () => {
Copy link
Member

@gziolo gziolo Oct 15, 2018

Copy and paste -> block

import { applyFormat } from './apply-format';

/**
* Toggle a format object to a Rich Text value at the current selection.
Copy link
Member

@gziolo gziolo Oct 15, 2018

Nit: it should start with Toggles

@ellatrix ellatrix requested a review from atimmer Oct 15, 2018
@gziolo
Copy link
Member

@gziolo gziolo commented Oct 15, 2018

I see the following error when hovering over the list of block transforms:

screen shot 2018-10-15 at 17 50 56

It looks like onHover isn't passed down properly to InserterList.

@gziolo
Copy link
Member

@gziolo gziolo commented Oct 16, 2018

Added buggy support for keyboard shortcuts with 5c052ce. For some reasons, you have to use keys combination twice to ensure it takes effect ...

@ellatrix
Copy link
Member Author

@ellatrix ellatrix commented Oct 18, 2018

Removing from the milestone as the parent issue is already listed.

@ellatrix ellatrix removed this from the 4.2 - API freeze milestone Oct 18, 2018
@ellatrix ellatrix mentioned this pull request Oct 18, 2018
Copy link
Member

@atimmer atimmer left a comment

This is really cool. The fact that one can write a format which is then usable across all blocks that use RichText is amazing.

The annotations API will work great on top of this. The only thing that's missing here is a way to have a format apply only to the editor, but not the serialization. But that could be added in the annotations PR.

@ellatrix ellatrix force-pushed the try/formatting-api-2 branch from 5c052ce to a688ac8 Oct 22, 2018
@ellatrix
Copy link
Member Author

@ellatrix ellatrix commented Oct 22, 2018

Added buggy support for keyboard shortcuts with 5c052ce. For some reasons, you have to use keys combination twice to ensure it takes effect ...

Now fixed in 6d7e50a.

Copy link
Member

@aduth aduth left a comment

@ellatrix ellatrix force-pushed the try/formatting-api-2 branch from 24ef98a to 7fd020f Oct 26, 2018
@ellatrix
Copy link
Member Author

@ellatrix ellatrix commented Oct 26, 2018

Let's merge and iterate!

@ellatrix ellatrix merged commit d95c7d6 into master Oct 26, 2018
2 checks passed
2 checks passed
@codecov[bot]
codecov/project 48.51% (-0.4%) compared to ac81cf7
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@ellatrix ellatrix deleted the try/formatting-api-2 branch Oct 26, 2018
daniloercoli added a commit that referenced this issue Oct 26, 2018
…rnmobile/merge-blocks-on-backspace

* 'master' of https://github.com/WordPress/gutenberg:
  Do not add isDirty prop to DOM (#11093)
  Format API (#10209)
  Remove 4.2 deprecated features (#10952)
  Update `@wordpress/hooks` README to include namespace mention (#11061)
  Feature: save lock control via actions (#10649)
  Fix usage of `preg_quote()` (#10998)
  Update plugin version to 4.1.1 (#11078)
  Improve preloading request code (#11007)
  Fix dynamic blocks not rendering in the frontend (#11050)
  Media & Text: Fixing vertical alignment of the image (#11025)
  Date: Mark getSettings as experimental (#10636)
  Improve handling of centered 1-col galleries with small images (#11040)
  Use better help text for ALT text input; fixes #8391. (#11052)

# Conflicts:
#	packages/editor/src/components/rich-text/index.native.js
@ellatrix ellatrix mentioned this pull request Oct 26, 2018
4 tasks
@gziolo gziolo mentioned this pull request Oct 26, 2018
7 tasks
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.

7 participants