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

Blocks: Add automatic handling of focus for RichText component #6419

Merged
merged 7 commits into from May 7, 2018

Conversation

@gziolo
Copy link
Member

gziolo commented Apr 25, 2018

Description

Closes #6213.

Follow up for #5029 where we introduced new context API from React 16.3 to apply isSelected prop to a few components used with BlockEdit.

This PR explores this idea further to make it possible to enforce that there can only be one RichText component focused at the same time. Historically it was manually enforced by using local state which stored which component is focused and prevented others to be selected at the same time.

I proposed the following

Yes, multiple RichText is quite difficult to handle and maintain. I will look into it next week. Maybe it wouldn't be that hard to automatically handle active state in a way where only one RichText can be active per block at a time.

@jorgefilipecosta also proposed together with @jayshenk the following:

I think we can have only one active RichText for the whole post. Maybe we can just use a component id and store id of RichText that is active, it would simplify the logic of some blocks.

I started small by using BlockEdit scoped solution to aovid dealing with the global store. I didn't see an immediate need to use wp.data to solve the issue we had here. We might want to revisit this later, but at the moment it seems like using the context API limits the number of operation React would have to perform where the focused element changes. I might be wrong here though.

How has this been tested?

Made sure it works without any changes for all blocks and for those updated in particular:

  • Pullquote
  • Quote
  • Text columns

Screenshots

auto-focus rich text

Types of changes

Refactoring, no changes in the UX or UI.

Next steps

We use a similar technique that uses withState to change focus for Gallery and Image blocks, but they are more complex so I excluded them from this PR. However we might look into exposing a general purpose helper that would allow to wrap components and inject this behavior to them.

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
@gziolo gziolo self-assigned this Apr 25, 2018
@gziolo gziolo force-pushed the update/rich-text-focus branch from 57280ac to 0e50419 Apr 25, 2018
@gziolo gziolo requested a review from mcsf Apr 25, 2018
@gziolo gziolo force-pushed the update/rich-text-focus branch from 0e50419 to c5a10a8 Apr 26, 2018
@gziolo gziolo changed the title [WIP] Blocks: Add automatic handling of focus for RichText component Blocks: Add automatic handling of focus for RichText component Apr 26, 2018
@gziolo

This comment has been minimized.

Copy link
Member Author

gziolo commented Apr 26, 2018

@zgordon @Shelob9 - this PR would solve the issue we had at WC Miami when you wanted to provide onFocus and isSelected to the RichText. Unlimited RichText components inside your block with automatically handled focus for you :)

@gziolo gziolo added this to the 2.8 milestone Apr 26, 2018
withInstanceId,
withBlockEditContext( ( context, ownProps ) => {
// When explicitly set as not selected, do nothing.
if ( ownProps.isSelected === false ) {

This comment has been minimized.

Copy link
@youknowriad

youknowriad Apr 27, 2018

Contributor

Should we add a deprecation message about the isSelected prop in RichText components?

This comment has been minimized.

Copy link
@gziolo

gziolo Apr 27, 2018

Author Member

We might, but as soon as we remove all occurrences in core blocks. Otherwise, we would flood the console with the deprecation messages. We should also make sure there aren't any cases when this possibility would still make sense.

This comment has been minimized.

Copy link
@Shelob9

Shelob9 Apr 27, 2018

Contributor

I love having focus handled automatically by default. I'd like to be able to optionally control it myself by setting that prop using my own logic.

Copy link
Contributor

youknowriad left a comment

I like it but there might be a small bug on initial mount.

Try this:

  • Insert a paragraph block
  • Using the autocompleter switch to the text columns block
  • Type and move the mouse
  • Notice the toolbar is not shown even if the text columns has the focus.
@gziolo

This comment has been minimized.

Copy link
Member Author

gziolo commented Apr 27, 2018

@youknowriad, great catch. I think I found a solution which solves the issue you described: f0edd3c. Let me know if you have some ideas how to improve this code. Maybe there is a way to avoid introducing another method in the BlockEdit context, which doesn't require passing down boolean param.

@gziolo gziolo changed the base branch from update/block-edit-context-name to master Apr 30, 2018
@gziolo gziolo force-pushed the update/rich-text-focus branch from f0edd3c to e80d22f Apr 30, 2018
@gziolo

This comment has been minimized.

Copy link
Member Author

gziolo commented Apr 30, 2018

Rebased with master.

return {
isBlockSelected: isSelected,
isSelected: context.isSelected && context.focusedElement === ownProps.instanceId,
onSetup: () => {

This comment has been minimized.

Copy link
@aduth

aduth Apr 30, 2018

Member

Nit: Why arrow function rather than function property shorthand?

onSetup() {
	// ...
}

This comment has been minimized.

Copy link
@gziolo

gziolo Apr 30, 2018

Author Member

Good question, I don’t have an answer 😜 I’ll update.

This comment has been minimized.

Copy link
@gziolo

gziolo May 2, 2018

Author Member

Updated with 6f4a136

return {
isBlockSelected: isSelected,
isSelected: context.isSelected && context.focusedElement === ownProps.instanceId,
onSetup: () => {

This comment has been minimized.

Copy link
@aduth

aduth Apr 30, 2018

Member

When does onSetup get called? When the user first focuses into the field? I would have thought it'd be at each's initialization, though it doesn't seem like we should want to set the focused element at all until the user actually focuses within (i.e. only onFocus).

This comment has been minimized.

Copy link
@gziolo

gziolo Apr 30, 2018

Author Member

OnSetup gets called when RichText gets mounted (initial render of the editor or when new block gets added). OnFocus is triggered only when a block gets focus but not on the initial render. That’s why we need to maintain two methods.

This comment has been minimized.

Copy link
@aduth

aduth May 2, 2018

Member

But I don't understand why we need to set the focused element merely by fact the RichText has initialized.

return {
isBlockSelected: isSelected,
isSelected: context.isSelected && context.focusedElement === ownProps.instanceId,
onSetup: () => {

This comment has been minimized.

Copy link
@aduth

aduth Apr 30, 2018

Member

Is this going to incur a lot of unnecessary renders due to the new function references being passed each time (bypassing any pure component behavior)?

This comment has been minimized.

Copy link
@gziolo

gziolo Apr 30, 2018

Author Member

There are also multiple props present that change frequently causing rerenders. I tried to add some optimizations to avoid creating new references, but it wasn’t helpful at all. I didn’t notice any improvements

This comment has been minimized.

Copy link
@aduth

aduth Apr 30, 2018

Member

"If you can't beat them, join them" ? 😆

This comment has been minimized.

Copy link
@gziolo

gziolo Apr 30, 2018

Author Member

It would be great to take a closer look at all updates triggered by RichText and better understand how and why it works this way.

@gziolo gziolo force-pushed the update/rich-text-focus branch from e80d22f to 6f4a136 May 2, 2018
@gziolo

This comment has been minimized.

Copy link
Member Author

gziolo commented May 2, 2018

I rebased with master again and also added a small change in 6f4a136.

@mcsf
mcsf approved these changes May 2, 2018
Copy link
Contributor

mcsf left a comment

Nice work.

@gziolo

This comment has been minimized.

Copy link
Member Author

gziolo commented May 2, 2018

@mcsf thanks for review. I want to investigate if we can use only onFocus set on the div element which wraps TinyMCE. Props to @aduth for suggesting this solution 👍

@aduth aduth modified the milestones: 2.8, 2.9 May 3, 2018
@gziolo gziolo force-pushed the update/rich-text-focus branch from 6f4a136 to c9f68d3 May 4, 2018
@gziolo

This comment has been minimized.

Copy link
Member Author

gziolo commented May 4, 2018

@aduth I think I found a way to simplify this implementation using onFocus attached to div as you suggested in one of the comments. It seems to work exactly the same but in addition we no longer create new functions on every re-render :)

}
// When explicitly set as selected or has onFocus prop provided,
// use value stored in the context instead.
if ( ownProps.isSelected === true || ownProps.onFocus ) {

This comment has been minimized.

Copy link
@aduth

aduth May 4, 2018

Member

With the onFocus prop effectively deprecated by #6286 / #6352, should we remove it from consideration here?

This comment has been minimized.

Copy link
@gziolo

gziolo May 4, 2018

Author Member

Yes, it makes sense 👍

edit: withState( {
editable: 'content',
} )( ( { attributes, setAttributes, isSelected, className, editable, setState } ) => {
edit: ( { attributes, setAttributes, isSelected, className } ) => {

This comment has been minimized.

Copy link
@aduth

aduth May 4, 2018

Member

Nit: Fewer characters as function property shorthand:

edit: ( { attributes, setAttributes, isSelected, className } ) => {
edit( { attributes, setAttributes, isSelected, className } ) {

This comment has been minimized.

Copy link
@gziolo

gziolo May 4, 2018

Author Member

Will do on Monday 👍

This comment has been minimized.

Copy link
@gziolo

gziolo May 7, 2018

Author Member

Also applied to Quote

@gziolo gziolo force-pushed the update/rich-text-focus branch from c9f68d3 to 8fb655e May 7, 2018
@gziolo gziolo merged commit ead8dc5 into master May 7, 2018
2 checks passed
2 checks passed
codecov/project 43.98% (-0.03%) compared to 5e940bf
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@gziolo gziolo deleted the update/rich-text-focus branch May 7, 2018
@ellatrix ellatrix mentioned this pull request Nov 1, 2018
0 of 4 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.