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

Jump to bottom

Add block warning overwrite #8166

Merged
merged 3 commits into from
Jan 18, 2019
Merged

Add block warning overwrite #8166

merged 3 commits into from
Jan 18, 2019

Conversation

johngodley
Copy link
Contributor

@johngodley johngodley commented Jul 24, 2018 •
edited
Loading

Description

Adds an option to the invalid block warning menu to fix the problem by re-creating the block with the current content, overwriting anything that is invalid. This was suggested in #7604

This is similar to the 'convert to blocks' option, but ensures the block stays of the same type. Contrast this with 'convert to blocks' which could convert to another block type, and could end in multiple blocks.

edit_post_ wordpress_latest _wordpress

In a lot of situations 'overwrite' and 'convert to blocks' will result in the same conversion so it's debatable whether this is a useful enough conversion to include but I'm raising it here for opinion. This is the same overwrite function that used to exist in older versions of Gutenberg.

How has this been tested?

  1. Add a paragraph block
  2. Edit block HTML and paste <p>this is a paragraph</p><blockquote>invalid content</blockquote>
  3. Deselect the block to trigger invalid block warning
  4. Pick 'Convert to blocks'
  5. Verify that the invalid block is converted to two blocks - a corrected paragraph and a blockquote
    edit_post_ wordpress_latest _wordpress
  6. Reset block HTML to (2) and pick 'Overwrite with valid block'
  7. Verify that the invalid block is converted to the first paragraph 'this is a paragraph', with the blockquote removed

Types of changes

New feature for invalid block warning. Should not affect anything else

Checklist:

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

@johngodley johngodley added [Status] In Progress Tracking issues with work in progress [Feature] Blocks Overall functionality of blocks labels Jul 30, 2018
@johngodley johngodley force-pushed the add/block-warning-overwrite branch 4 times, most recently from f3204d3 to e6ff955 Compare August 14, 2018 05:29
@johngodley johngodley mentioned this pull request Aug 28, 2018
Open
@johngodley johngodley force-pushed the add/block-warning-overwrite branch 3 times, most recently from 02c6ba8 to 654100d Compare August 31, 2018 11:48
@johngodley johngodley changed the title WIP - Add block warning overwrite Add block warning overwrite Aug 31, 2018
@johngodley johngodley removed the [Status] In Progress Tracking issues with work in progress label Aug 31, 2018
jorgefilipecosta
jorgefilipecosta reviewed Sep 14, 2018
View reviewed changes
Copy link
Member

@jorgefilipecosta jorgefilipecosta left a comment

Choose a reason for hiding this comment

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

Hi @johngodley, that sounds like a good feature to have 👍 I tried to test this and I'm having some problems. Each time I press the action to overwrite nothing happens.
sep-14-2018 16-24-29
Am I doing something wrong?

@johngodley
Copy link
Contributor Author

Uhh, sorry - my merge conflict resolving went awry. I've fixed the stupid typo and it should now do something!

jorgefilipecosta
jorgefilipecosta approved these changes Oct 10, 2018
View reviewed changes
Copy link
Member

@jorgefilipecosta jorgefilipecosta left a comment

Choose a reason for hiding this comment

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

I rebased this PR did some tests things seem fine. I only found a problem with blocks with nested children, I provided a possible code change that seems to fix the problem.
This feature is really useful during block development. Feel free to merge it 👍

packages/editor/src/components/block-list/block-invalid-warning.js Outdated
@@ -90,6 +91,7 @@ const blockToBlocks = ( block ) => rawHandler( {
HTML: block.originalContent,
mode: 'BLOCKS',
} );
const blockOverwrite = ( { name, attributes } ) => createBlock( name, attributes );
Copy link
Member

Choose a reason for hiding this comment

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

I checked that on blocks with nested children we delete all the children content and we just restart the innerBlocks area.
That can be tested with block https://gist.github.com/jorgefilipecosta/7f0a54193f9d8d28c1514aa2e57a00fc.
this seems fixable with the following line:

const blockOverwrite = ( { name, attributes, innerBlocks } ) => createBlock( name, attributes, innerBlocks );

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice, thanks!

@chrisvanpatten
Copy link
Member

I wonder if this could be renamed to something like "Try to recover this block" or something? "Overwrite with valid block" isn't entirely clear (the "valid" bit in particular might be confusing).

Love the idea though!

@johngodley
Copy link
Contributor Author

Good point. I've changed the text to say 'Attempt block recovery'. If that still makes sense then I'll merge

@jorgefilipecosta
Copy link
Member

Pinging @karmatosed regarding the user experience/message. From the code point of view everything looks fine 👍

@jorgefilipecosta jorgefilipecosta added the Needs Design Feedback Needs general design feedback. label Oct 11, 2018
@karmatosed
Copy link
Member

Attempt works for me :)

@karmatosed karmatosed removed the Needs Design Feedback Needs general design feedback. label Nov 22, 2018
@karmatosed karmatosed added this to the WordPress 5.0.1 milestone Nov 22, 2018
@mtias mtias modified the milestones: WordPress 5.0.1, 4.8 Dec 1, 2018
@fastlinemedia fastlinemedia mentioned this pull request Dec 13, 2018
Closed
@youknowriad youknowriad removed this from the 4.8 milestone Dec 19, 2018
@youknowriad youknowriad added this to the 4.9 milestone Dec 19, 2018
@youknowriad
Copy link
Contributor

Looks like this needs a rebase. Let's rebase and merge.

@johngodley @jorgefilipecosta
Invalid block: add an ‘overwrite’ option
fc33046 
This converts the invalid block to a single block of the same type, with any invalid HTML corrected by the block creation process
@johngodley @jorgefilipecosta
Fix typo
1a70aae
@johngodley @jorgefilipecosta
Tweak block recovery text
0db0d39 
Also support inner blocks
@jorgefilipecosta jorgefilipecosta merged commit 48598b9 into master Jan 18, 2019
@jorgefilipecosta jorgefilipecosta deleted the add/block-warning-overwrite branch January 18, 2019 14:13
youknowriad pushed a commit that referenced this pull request Mar 6, 2019
@johngodley @youknowriad
Add block warning overwrite (#8166)
d30edfa 
## Description
Adds an option to the invalid block warning menu to fix the problem by re-creating the block with the current content, overwriting anything that is invalid. This was suggested in #7604

This is similar to the 'convert to blocks' option, but ensures the block stays of the same type. Contrast this with 'convert to blocks' which could convert to another block type, and could end in multiple blocks.

![edit_post_ _wordpress_latest_ _wordpress](https://user-images.githubusercontent.com/1277682/44906590-3dee2f00-ad0d-11e8-9a3a-7b2113aebde1.jpg)

In a lot of situations 'overwrite' and 'convert to blocks' will result in the same conversion so it's debatable whether this is a useful enough conversion to include but I'm raising it here for opinion. This is the same overwrite function that used to exist in older versions of Gutenberg.

## How has this been tested?

1. Add a paragraph block
2. Edit block HTML and paste `<p>this is a paragraph</p><blockquote>invalid content</blockquote>`
3. Deselect the block to trigger invalid block warning
4. Pick 'Convert to blocks'
5. Verify that the invalid block is converted to two blocks - a corrected paragraph and a blockquote
![edit_post_ _wordpress_latest_ _wordpress](https://user-images.githubusercontent.com/1277682/44906652-6e35cd80-ad0d-11e8-9652-af1cbc19ac1f.jpg)
6. Reset block HTML to (2) and pick 'Overwrite with valid block'
7. Verify that the invalid block is converted to the first paragraph 'this is a paragraph', with the blockquote removed

## Types of changes
New feature for invalid block warning. Should not affect anything else

## Checklist:
- [ ] My code is tested.
- [ ] My code follows the WordPress code style. <!-- Check code: `npm run lint`, Guidelines: https://make.wordpress.org/core/handbook/best-practices/coding-standards/javascript/ -->
- [ ] My code follows the accessibility standards. <!-- Guidelines: https://make.wordpress.org/core/handbook/best-practices/coding-standards/accessibility-coding-standards/ -->
- [ ] My code has proper inline documentation. <!-- Guidelines: https://make.wordpress.org/core/handbook/best-practices/inline-documentation-standards/javascript/ -->
youknowriad pushed a commit that referenced this pull request Mar 6, 2019
@johngodley @youknowriad
Add block warning overwrite (#8166)
3ffbfb5 
## Description
Adds an option to the invalid block warning menu to fix the problem by re-creating the block with the current content, overwriting anything that is invalid. This was suggested in #7604

This is similar to the 'convert to blocks' option, but ensures the block stays of the same type. Contrast this with 'convert to blocks' which could convert to another block type, and could end in multiple blocks.

![edit_post_ _wordpress_latest_ _wordpress](https://user-images.githubusercontent.com/1277682/44906590-3dee2f00-ad0d-11e8-9a3a-7b2113aebde1.jpg)

In a lot of situations 'overwrite' and 'convert to blocks' will result in the same conversion so it's debatable whether this is a useful enough conversion to include but I'm raising it here for opinion. This is the same overwrite function that used to exist in older versions of Gutenberg.

## How has this been tested?

1. Add a paragraph block
2. Edit block HTML and paste `<p>this is a paragraph</p><blockquote>invalid content</blockquote>`
3. Deselect the block to trigger invalid block warning
4. Pick 'Convert to blocks'
5. Verify that the invalid block is converted to two blocks - a corrected paragraph and a blockquote
![edit_post_ _wordpress_latest_ _wordpress](https://user-images.githubusercontent.com/1277682/44906652-6e35cd80-ad0d-11e8-9652-af1cbc19ac1f.jpg)
6. Reset block HTML to (2) and pick 'Overwrite with valid block'
7. Verify that the invalid block is converted to the first paragraph 'this is a paragraph', with the blockquote removed

## Types of changes
New feature for invalid block warning. Should not affect anything else

## Checklist:
- [ ] My code is tested.
- [ ] My code follows the WordPress code style. <!-- Check code: `npm run lint`, Guidelines: https://make.wordpress.org/core/handbook/best-practices/coding-standards/javascript/ -->
- [ ] My code follows the accessibility standards. <!-- Guidelines: https://make.wordpress.org/core/handbook/best-practices/coding-standards/accessibility-coding-standards/ -->
- [ ] My code has proper inline documentation. <!-- Guidelines: https://make.wordpress.org/core/handbook/best-practices/inline-documentation-standards/javascript/ -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jorgefilipecosta jorgefilipecosta jorgefilipecosta approved these changes

Assignees
No one assigned
Labels
[Feature] Blocks Overall functionality of blocks
Projects
None yet
Milestone
4.9 (Gutenberg)
Development

Successfully merging this pull request may close these issues.
6 participants
@johngodley @chrisvanpatten @jorgefilipecosta @karmatosed @youknowriad @mtias