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

Fix: Blocks: Make backspace behavior of quote, verse and performated consistent #7958

Conversation

@jorgefilipecosta
Copy link
Member

jorgefilipecosta commented Jul 13, 2018

In list, heading, and paragraph if we had a paragraph before and pressed backspace the contents of the blocks would be merged with the paragraph. If we had a block of the same type before, the contents would be merged on backspace. And if had a paragraph after the block and pressed backspace there the contents of the paragraph would be appended to the contents of the block.

These behaviors make sense but on the quote, verse and performated they were not available.

This PR adds simple merge functions to each of the three blocks and passes the onMerge handler to the RichText component they use.

Fixes: #5057

How has this been tested?

For each of the 3 blocks ( quote, verse and performated ) I made the following stepts:

  • Added two blocks of the same type with some content. Pressed backspace on the second block and verified the blocks merged.
  • Added a paragrah with content and one of the blocks after pressed backspace on the start of the block and verified the contents of the block merged with the paragraph.
  • Added a block and a paragraph pressed backspace at the start of the paragraph and verified that the contents of the paragraph joined the block.
@aduth aduth force-pushed the fix/make-backspace-consistent-on-quote-verse-preformated branch from 7bee586 to 2cf13aa Sep 13, 2018
Copy link
Member

aduth left a comment

This would be good to get in.


merge( attributes, attributesToMerge ) {
return {
content: concatChildren( attributes.content, attributesToMerge.content ),

This comment has been minimized.

Copy link
@aduth

aduth Sep 13, 2018

Member

We should use wp.blocks.node.concat

merge( attributes = {}, attributesToMerge = {} ) {
return {
...attributes,
value: ( attributes.value || [] ).concat( attributesToMerge.value || [] ),

This comment has been minimized.

Copy link
@aduth

aduth Sep 13, 2018

Member
  • Why not concat like we did in the other block?
  • Is a value ever not an array?

This comment has been minimized.

Copy link
@jorgefilipecosta

jorgefilipecosta Sep 18, 2018

Author Member

Why not concat like we did in the other block?

The reason for the difference between the quote and other blocks is because the quote can contain multiple paragraphs (fields of text). So for the quote, I think we should concat to the existing list of paragraphs.

Is a value ever not an array?

I think the value is always an array, but as I was in doubts, I kept the checks. Now I did some tests with code assuming the value is always an array and things worked great, so I think it is safe to consider the value is always an array (and it should be unless there is a bug in another part).

@aduth

This comment has been minimized.

Copy link
Member

aduth commented Sep 13, 2018

Oh, I should mention that I did a rebase to resolve conflicts and force-pushed.

@jorgefilipecosta jorgefilipecosta force-pushed the fix/make-backspace-consistent-on-quote-verse-preformated branch from 2cf13aa to 92f6c68 Sep 18, 2018
@jorgefilipecosta

This comment has been minimized.

Copy link
Member Author

jorgefilipecosta commented Sep 18, 2018

Hi @aduth thank you for the review and for rebasing this old PR 👍 I applied the feedback you provided!

@jorgefilipecosta jorgefilipecosta added this to the 4.0 milestone Sep 18, 2018
@aduth
aduth approved these changes Sep 18, 2018
@@ -233,6 +233,14 @@ export const settings = {
);
},

merge( attributes = {}, attributesToMerge = {} ) {

This comment has been minimized.

Copy link
@aduth

aduth Sep 18, 2018

Member

When will attributes or attributesToMerge ever be undefined here?

This comment has been minimized.

Copy link
@jorgefilipecosta

jorgefilipecosta Sep 18, 2018

Author Member

Corrected 👍

…consistent with what we had in paragraph, heading, and list.

In list, heading, and paragraph if we had a paragraph before and pressed backspace the contents of the blocks would be merged with the paragraph. If we had a block of the same type before, the contents would be merged on backspace. And if had a paragraph after the block and pressed backspace there the contents of the paragraph would be appended to the contents of the block.

These behaviors make sense but on the quote, verse and performated they were not available.

This PR adds simple merge functions to each of the three blocks and passes the onMerge handler to the RichText component they use.
@jorgefilipecosta jorgefilipecosta force-pushed the fix/make-backspace-consistent-on-quote-verse-preformated branch from 92f6c68 to 50c81e4 Sep 18, 2018
@jorgefilipecosta jorgefilipecosta merged commit e0e6668 into master Sep 18, 2018
2 checks passed
2 checks passed
codecov/project 48.77% (-0.02%) compared to bd5910b
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@jorgefilipecosta jorgefilipecosta deleted the fix/make-backspace-consistent-on-quote-verse-preformated branch Sep 18, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.