Skip to content

Quote Block: Fixing focus transfer between citation and content#5100

Merged
youknowriad merged 2 commits intomasterfrom
fix/quote-focus
Feb 16, 2018
Merged

Quote Block: Fixing focus transfer between citation and content#5100
youknowriad merged 2 commits intomasterfrom
fix/quote-focus

Conversation

@youknowriad
Copy link
Copy Markdown
Contributor

closes #5081

This PR fixes moving focus between citation and content in quote blocks.

Testing instructions

  • Add a quote block
  • type content
  • type citation
  • click to switch focus between quote and citation

@youknowriad youknowriad self-assigned this Feb 16, 2018
this.props.value !== this.savedContent
this.props.value !== this.savedContent &&
! isEqual( this.props.value, prevProps.value ) &&
! isEqual( this.props.value, this.savedContent )
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This is the real fix. I removed this as part of #4955 but it's necessary to avoid unnecessarily content updates. I don't see any performance loss in adding this and it could even improve performance because we don't use those TinyMCE functions setBookmark and setContent constantly

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@youknowriad I feel like we removed and added this a few times I the last year now. :) Maybe worth leaving a comment?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

(I was surprised to see these lines again.)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good call 👍


.wp-block-pullquote {
cite {
display: block;
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This just increases the click area of the citation to the whole line (width: 100%)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This kind of tells me that we shouldn't be using cite for this... :)

@youknowriad youknowriad added this to the 2.2 milestone Feb 16, 2018
@aduth
Copy link
Copy Markdown
Member

aduth commented Feb 16, 2018

Seeing an issue:

  1. Type a paragraph
  2. Press enter

Content from the first paragraph disappears.

@jorgefilipecosta
Copy link
Copy Markdown
Member

It seems like this solved the problems with the quote. Regarding the problem reported by @aduth I'm also able to replicate it seems to only happen in paragraph other blocks e.g: list works well.

@youknowriad
Copy link
Copy Markdown
Contributor Author

Good catch @aduth It's fixed now.

It was happening because when you hit "Enter", TinyMCE updates the content of the rich text before even we call onSplit. And since we want to keep the same content as before in this case, we have to call updateContent to make sure the "Enter" key is reverted properly.

Previous updateContent was called on rerender because the value prop was failing the strict equality check.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If it's the case that we must call updateContent before every this.props.onSplit, could be wise to create a separate class function for consistency/safety's sake.

*/
restoreContentAndSplit( before, after, blocks ) {
this.updateContent();
this.props.onSplit( before, after, ...blocks );
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hm, now we have (1) this.props.onSplit checks in several places and (2) none here.

}

/**
* Calling onSplit means we need to abort the change done by TinyMCE.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I don't really understand this function. In most places where it's used, we're also calling preventDefault. Shouldn't that be enough to "abort the change" that would otherwise be effected by TinyMCE ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Quote: impossible to switch between fields by clicking on text

4 participants