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

Add hook to validate useOnce blocks #5031

Merged
merged 1 commit into from
Feb 24, 2018
Merged

Conversation

mcsf
Copy link
Contributor

@mcsf mcsf commented Feb 13, 2018

Description

Fixes #4932.

This pull request is one answer to handling scenarios in which multiple instances of a block are found in a post even though the block's type specifies it should only be used once (useOnce). The following example should be useful:

Edit: The Edit as HTML button has been removed for now, and the More block doesn't offer any transformation (offering "Next Page" was merely conceptual). Instead, a Find original button was added that selects (and moves into view) the one block in the content that is valid for the same block type.

Source Result
screen shot 2018-02-13 at 14 22 26 screen shot 2018-02-13 at 14 22 05

Caveat

  • The inspector controls aren't disabled for invalid blocks. This is a limitation stemming from the chosen hook (blocks.BlockEdit) and can be revisited.
  • The suggestion to convert More to a Next Page block came out of my own liberty. We do not provide such a block yet, but it seemed like it would be a very timely suggestion: a lot of users may not even know of <!--nextpage--> and may be stitching together a post and realize they have multiple break points that they were representing with <!--more-->. Creating blocks/library/next-page and adding a transform from More would solve this. Edit: more on Pagination below.

How Has This Been Tested?

  • Source of a post with multiple Subhead and More blocks.
  • Very rough, needs testing, UX and design feedback.
  • Needs a couple of unit tests.

Screenshots (jpeg or gifs if applicable):

gutenberg-use-once

Types of changes

Enhancement; extension.

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code has proper inline documentation.

@mcsf mcsf added [Feature] Block API API that allows to express the block paradigm. [Feature] Blocks Overall functionality of blocks Needs Design Feedback Needs general design feedback. labels Feb 13, 2018
@mcsf mcsf added the [Type] Enhancement A suggestion for improvement. label Feb 13, 2018
@mcsf
Copy link
Contributor Author

mcsf commented Feb 13, 2018

Adding that the current take is pretty opinionated in that it requires the user to take action. My first stab included a Ignore option. I’m not sure where I stand on honoring this contract between block author and block user. It could be argued that offering transforms + conversion to HTML is a way to let the user keep their content and not restrict them too much.

@mcsf
Copy link
Contributor Author

mcsf commented Feb 13, 2018

On the Pagination block (nextpage), there a couple of past issues for it. Linking to the most recent issue (#4930) and an open PR (#1467). Furthermore, noting that work should be performed in accordance with #2973.

@mcsf mcsf mentioned this pull request Feb 13, 2018
@hedgefield
Copy link

I like your approach of giving the user options instead of locking them down with "this is wrong". That's a good UX. And the transformations sound logical, so for me this is a good solution to #4932.

@mcsf mcsf added this to the 2.3 milestone Feb 21, 2018
@mcsf mcsf force-pushed the try/validate-use-once-blocks branch 2 times, most recently from 0e0a75a to 375dfc1 Compare February 24, 2018 15:58
@mcsf mcsf force-pushed the try/validate-use-once-blocks branch from 375dfc1 to 902f982 Compare February 24, 2018 17:20
@mtias
Copy link
Member

mtias commented Feb 24, 2018

Looks good to me.

@mcsf
Copy link
Contributor Author

mcsf commented Feb 24, 2018

I'm going to merge this. Noting that more of Gutenberg's core validation process could be moved to a hook like this. Until then, I want this atom merged so it can serve as yet another example of usage of hooks.

@mcsf mcsf merged commit 07dbbb0 into master Feb 24, 2018
@mcsf mcsf deleted the try/validate-use-once-blocks branch February 24, 2018 18:08
@mcsf mcsf added the [Feature] Extensibility The ability to extend blocks or the editing experience label Feb 27, 2018
@hedgefield
Copy link

hedgefield commented Mar 7, 2018

@mcsf Ah, noticed a bug with this: when adding the subheading block more than once, it shows the "you cannot use this block more than once" as expected. BUT, you can then transform that block into a paragraph, and it still stays locked, saying "Paragraph can not be used more than once" 😄

@mcsf
Copy link
Contributor Author

mcsf commented Mar 7, 2018

@hedgefield: interesting. I can't repro, though. Could you perhaps provide a screencast?

@hedgefield
Copy link

hedgefield commented Mar 7, 2018

Can't repro on master either (I just get two subhead blocks, no warning), but this is what happens on 2.3.0 for my colleague.

tim

Thinking about it more, not sure if this is your code anymore, that was limited to the code editor I think? Shall I report this as a bug?

@mcsf
Copy link
Contributor Author

mcsf commented Mar 7, 2018

Ah, good catch. I can repro. Yep, you can make an issue, thanks!

@hedgefield
Copy link

👍 #5471

// Otherwise, only pass `originalBlockUid` if it refers to a different
// block from the current one.
const firstOfSameType = find( blocks, ( { name } ) => block.name === name );
const isInvalid = firstOfSameType && firstOfSameType.uid !== block.id;
Copy link
Member

Choose a reason for hiding this comment

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

Is this actually doing anything? Where is block.id assigned for this? Is this a typo for block.uid?

(Asking as I seek to upgrade all references of uid to clientId)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's passed from BlockListBlock to BlockEdit, but it's a synonym of clientId.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

<BlockEdit
name={ blockName }
isSelected={ isSelected }
attributes={ block.attributes }
setAttributes={ this.setAttributes }
insertBlocksAfter={ isLocked ? undefined : this.insertBlocksAfter }
onReplace={ isLocked ? undefined : onReplace }
mergeBlocks={ isLocked ? undefined : this.mergeBlocks }
id={ block.uid }
isSelectionEnabled={ this.props.isSelectionEnabled }
toggleSelection={ this.props.toggleSelection }
/>

Copy link
Member

Choose a reason for hiding this comment

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

Another reason #7990 was desperately needed 😅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Block API API that allows to express the block paradigm. [Feature] Blocks Overall functionality of blocks [Feature] Extensibility The ability to extend blocks or the editing experience Needs Design Feedback Needs general design feedback. [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants