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

RichText: add withoutInteractiveFormatting prop #14542

Merged
merged 7 commits into from Jul 24, 2019

Conversation

@ellatrix
Copy link
Member

commented Mar 20, 2019

Description

Fixes #15212 (comment).
Fixes #14528.
Fixes #15858.

Adds a withoutInteractiveFormatting prop to RichText. We currently have a use case where we'd like to remove the link button (and any other interactive elements) from blocks such as button, file, and search, as it would create invalid HTML.

Also deprecates formattingControls and introduces allowedFormats (similar to allowedBlocks). Example: [ 'core/bold' ].

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.
  • I've included developer documentation if appropriate.

@ellatrix ellatrix force-pushed the try/rich-text-disallow-formats branch from 1986755 to f959e91 Apr 4, 2019

@ellatrix ellatrix requested a review from aduth as a code owner Apr 4, 2019

@aduth

This comment was marked as outdated.

Copy link
Member

commented May 22, 2019

Is there consensus enough to move forward here? Is it just in need of a code review? (Also a rebase, it seems)

@noisysocks
Copy link
Member

left a comment

Flipping this API makes total sense to me! 👍

I think we need to make the new prop more backwards compatible. Other than that, this is great.

packages/block-library/src/file/edit.js Outdated Show resolved Hide resolved
packages/block-library/src/file/edit.js Outdated Show resolved Hide resolved
packages/block-library/src/search/edit.js Outdated Show resolved Hide resolved
packages/block-library/src/search/edit.js Outdated Show resolved Hide resolved
@ellatrix ellatrix referenced this pull request Jun 26, 2019
5 of 5 tasks complete

@ellatrix ellatrix force-pushed the try/rich-text-disallow-formats branch from f959e91 to e674f20 Jun 26, 2019

@ellatrix ellatrix added the Regression label Jun 26, 2019

@ellatrix ellatrix added this to the Gutenberg 6.1 milestone Jun 26, 2019

@ellatrix ellatrix changed the title RichText: add disallowFormats prop RichText: add withoutInteractiveFormatting prop Jun 26, 2019

I'm not sure if there's a way to make it fully backwards compatible.

@noisysocks

This comment was marked as outdated.

Copy link
Member

commented Jul 4, 2019

Checking back in on this! 🙂 My understanding is that we're removing the formattingControls prop and replacing it with withoutInteractiveFormatting? Can we update the RichText README.md to reflect this?

@ellatrix

This comment was marked as outdated.

Copy link
Member Author

commented Jul 5, 2019

@noisysocks Thanks for bringing that up! Forgot about it. :) The file is now updated.

@ellatrix ellatrix requested review from youknowriad and noisysocks Jul 5, 2019

@ellatrix

This comment has been minimized.

Copy link
Member Author

commented Jul 11, 2019

@chrisvanpatten I'm trying to understand the motivation behind this a bit more.

  • Why do you want to restrict the formatting? Just because?
  • Why remove italics but not bold?
  • Would you say this in more something you'd want to restrict on a document level, rather than on a block level?
@chrisvanpatten

This comment has been minimized.

Copy link
Member

commented Jul 11, 2019

@ellatrix My company owns a number of major consumer editorial brands, and we have design teams, product management, and other stakeholders determining what functionality our editorial teams and curation teams should have access to… or not.

In the case I noted above, we are allowing our editorial teams to insert "headlines" for given blocks, and want to provide a nice "WYSIWYG" preview of those headlines. We also want them to be able to bold or link arbitrary text selections within the headline.

The reason we would not want to allow other formats is because the headlines are pre-styled by our design teams, and are intended to conform to internal guidelines.

In other cases there might be spots where we want to only allow links (for example we have a custom "link list" block that requires this exact use-case), or maybe we only want to support a particular custom formatting option.

Ultimately, the RichText block gives us an awesome visual interface, with the "WYSIWYG"-style preview, toolbar buttons, etc. and we want to keep that behavior. But it's essential that we are able to dictate exactly which formatting options are possible in an individual instance of the RichText component.

Document-level settings here would not be enough.

@ellatrix

This comment has been minimized.

Copy link
Member Author

commented Jul 11, 2019

Ok. I'm just wondering how this would work if someone wants certain controls on a block that they've not implemented. E.g. if your company wants limited formatting buttons on headings, which is implemented by core. Or if a plugin wants to change the formatting buttons on a block another plugin created. By setting the formatting controls to known formats only, there is no way to extend it.

@chrisvanpatten

This comment has been minimized.

Copy link
Member

commented Jul 11, 2019

Ok. I'm just wondering how this would work if someone wants certain controls on a block that they've not implemented. E.g. if your company wants limited formatting buttons on headings, which is implemented by core. Or if a plugin wants to change the formatting buttons on a block another plugin created. By setting the formatting controls to known formats only, there is no way to extend it.

I wouldn't ever expect to be able to change the formatting buttons on a block that I hadn't created. I mean, that would be cool — but we're pretty much never using core blocks, and instead have been re-implementing our own. I think it's generally understood that modifying block behavior is always a bit fraught.

I would only want this control at a component level, if I'm directly inserting a <RichText /> somewhere in the UI, within a block or component I control.

@ellatrix

This comment has been minimized.

Copy link
Member Author

commented Jul 12, 2019

@chrisvanpatten Sure, it makes sense to allow it if you know what you're doing. I'll add something like allowedFormats (just like allowedBlocks) where you can fine tune the controls. I'll deprecate formattingControls and make it backwards compatible (even though it's been a bit broken for a while).

@chrisvanpatten

This comment has been minimized.

Copy link
Member

commented Jul 12, 2019

@chrisvanpatten Sure, it makes sense to allow it if you know what you're doing. I'll add something like allowedFormats (just like allowedBlocks) where you can fine tune the controls. I'll deprecate formattingControls and make it backwards compatible (even though it's been a bit broken for a while).

@ellatrix you da best! That would be amaaaaazing. Thank you!!!

@ellatrix ellatrix requested a review from epiqueras Jul 20, 2019

@ellatrix ellatrix requested a review from chrisvanpatten Jul 20, 2019

@ellatrix

This comment has been minimized.

Copy link
Member Author

commented Jul 20, 2019

@chrisvanpatten @epiqueras I added a allowedFormats prop now. This would also remove the dropdown if there's no formats.

@ellatrix

This comment has been minimized.

Copy link
Member Author

commented Jul 20, 2019

We will also need this PR for footnotes. The footnotes format will be disabled in instances that enabled withoutInteractiveFormatting (e.g. button block).

@ellatrix

This comment has been minimized.

Copy link
Member Author

commented Jul 20, 2019

@mcsf Might be useful for you title block. :)

@obenland

This comment has been minimized.

Copy link
Member

commented Jul 22, 2019

Got here through the "Good First Review" label and thought it was added by accident or a long time ago, given the size of this PR. My bad, I put it back :)

@noisysocks
Copy link
Member

left a comment

Looks great, and I like this API! 👍

We ought to add a note to block-editor/CHANGELOG.md which describes this change.

While testing this I noticed a bug where, in the Search block, if you move focus from the label field to the placeholder field, the formatting controls don't go away. This bug exists on master, though, so not a blocker.

search


if ( formattingControls ) {
deprecated( 'wp.blockEditor.RichText formattingControls prop', {
version: 'the future',

This comment has been minimized.

Copy link
@noisysocks

noisysocks Jul 23, 2019

Member

Unless there's a specific version that we're planning to remove this, I think it's best to omit the version option.

Suggested change
version: 'the future',

This comment has been minimized.

Copy link
@ellatrix

ellatrix Jul 24, 2019

Author Member

I did what the docs recommended:

* deprecated( 'Eating meat', {
* version: 'the future',
* alternative: 'vegetables',
* plugin: 'the earth',
* hint: 'You may find it beneficial to transition gradually.',
* } );

return;
}

if ( formattingControls ) {

This comment has been minimized.

Copy link
@noisysocks

noisysocks Jul 23, 2019

Member

This if isn't necessary if we move the deprecated() call to be after the if ( allowedFormats ) { ... } below.

This comment has been minimized.

Copy link
@ellatrix

ellatrix Jul 24, 2019

Author Member

What if you use both?

@ellatrix

This comment has been minimized.

Copy link
Member Author

commented Jul 24, 2019

While testing this I noticed a bug where, in the Search block, if you move focus from the label field to the placeholder field, the formatting controls don't go away. This bug exists on master, though, so not a blocker.

That only happens when switching between a RichText instance and input or textarea, right? I know that this happens, we keep some selection state to work around losing it when you press a button outside the editable area. I'll try to fix it at some point.

@ellatrix ellatrix force-pushed the try/rich-text-disallow-formats branch from 05791a9 to d959422 Jul 24, 2019

@ellatrix ellatrix merged commit 4ef9b7e into master Jul 24, 2019

1 of 2 checks passed

Filter merged Filter merged
Details
Travis CI - Pull Request Build Passed
Details

@ellatrix ellatrix deleted the try/rich-text-disallow-formats branch Jul 24, 2019

@youknowriad

This comment has been minimized.

Copy link
Contributor

commented Jul 25, 2019

Does this fix #12439 too?

sbardian added a commit to sbardian/gutenberg that referenced this pull request Jul 29, 2019

RichText: add withoutInteractiveFormatting prop (WordPress#14542)
* Add withoutInteractiveFormatting

* Update docs

* Introduce allowedFormats

* Add readme entry

* Deprecate formattingControls

* Oops

* Address feedback
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.