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

Display block boundaries when cursor moves while typing #1149

Merged
merged 3 commits into from
Jun 13, 2017

Conversation

aduth
Copy link
Member

@aduth aduth commented Jun 12, 2017

Closes #657

This pull request seeks to redisplay block boundaries if the user is currently typing within a block and then moves their cursor within said block. This is the final remaining task noted in #657.

Testing instruction:

  1. Navigate to Gutenberg Demo editor
  2. Select a block using mouse
  3. Begin typing
  4. Note that boundaries fade
  5. Move cursor
  6. Note that block boundaries reappear

@aduth aduth added [Feature] Blocks Overall functionality of blocks General Interface Parts of the UI which don't fall neatly under other labels. labels Jun 12, 2017
@youknowriad
Copy link
Contributor

What if we start moving the mouse elsewhere (outside the block boundaries), should we stop typing too?

@aduth
Copy link
Member Author

aduth commented Jun 13, 2017

@youknowriad I could see both arguments:

  • That moving the mouse over the block is similar to the "hover" highlight, except the block's already selected so we show all controls.
  • That moving the mouse is an intent that "you're done typing for now" regardless of where the cursor is

I'm more leaning toward the second of these two options.

cc @jasmussen

@jasmussen
Copy link
Contributor

I'm leaning to the latter also.

@aduth aduth force-pushed the update/657-block-boundaries-mouse-move branch from eae6403 to 0b069fd Compare June 13, 2017 18:55
@aduth
Copy link
Member Author

aduth commented Jun 13, 2017

Rebased to resolve conflicts and pushed 0b069fd0c0c14fa088e5164f11263c0b4ced455f which binds mousemove to entire document while block is being typed within, per discussion.

The block.js component is becoming increasingly unwieldy but I chose to add it here because some amount of typing behavior is necessary anyways (e.g. whether to show controls). Other consideration I had was a <BlockTypingTracker /> component to wrap <BlockEdit /> rendering and handle the events, or perhaps as a better fit for decorator type behavior proposed in #1023.

@@ -104,6 +147,10 @@ class VisualEditorBlock extends wp.element.Component {
}
}

stopTyping() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we could avoid this function

Copy link
Member Author

@aduth aduth Jun 13, 2017

Choose a reason for hiding this comment

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

You mean binding this.props.onStopTying as the event handler? It's a little concerning to me since the onStopTyping prop may not be the same in the constructor as it is when the component later unmounts (or the event is bound), so we risk zombie event listeners. I'm actually unsure of the behavior of react-redux's mapDispatchToProps. Seems like it should only need to generate the props once when initializing, so it's likely not a concern in reality, just in theory.

Copy link
Contributor

@youknowriad youknowriad left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@aduth aduth force-pushed the update/657-block-boundaries-mouse-move branch from 0b069fd to 6d4f092 Compare June 13, 2017 20:17
@aduth aduth merged commit 4c4f96d into master Jun 13, 2017
@aduth aduth deleted the update/657-block-boundaries-mouse-move branch June 13, 2017 20:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Blocks Overall functionality of blocks General Interface Parts of the UI which don't fall neatly under other labels.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants