UI: Adding the selected and hover state for the visual editor #360
Conversation
youknowriad
added
the
Framework
|
@aduth The |
|
Love it! 👍 One thing — if you click a block, leave the mouse where you clicked, and start typing, the block gets unselected but it still shows up as hovered. We should fade out everything when you are typing. If that's an easy fix, would be nice to get in this PR, otherwise feel free to merge this and let's revisit later. |
@jasmussen When should we trigger the |
|
@jasmussen I updated and dropped the "hover" state when we start typing and I hover again when we start moving the mouse again. |
|
Sorry missed your first ping. Just tried this. Works GREAT. Exactly how I think it should work. Ship it! |
editor/editor/mode/visual.js
Outdated
| ...blocks.slice( index + 1 ) | ||
| ]; | ||
|
|
||
| this.setState( { blocks: newBlocks } ); |
There was a problem hiding this comment.
Can we avoid the mix of state and props blocks by forcing the component to be used as a controlled component? i.e. all blocks changes go through this.props.onChange and we assume that this.props.blocks is the canonical set of blocks.
There was a problem hiding this comment.
It was originally like that But I don't know, I think the blocks, selected, hovered and more to come should stay on the same object (The visual editor's state) which could be updated using a "command" pattern (similar to redux) (See the updater in the per-block prototypes.
This allows us to handle undo/redo easily for example. Also having the selected, hovered state values on the parent component is not a good option because they're not needed in the text mode.
Thoughts?
There was a problem hiding this comment.
This feels similar to what I'd remarked at #348 (comment) , specifically that it becomes a maintenance nightmare to be managing multiple sources of truth, in this case reconciling the blocks of a component's state and the blocks passed from a parent component.
There was a problem hiding this comment.
For me neither of the two solutions is perfect. Time to introduce redux ?
There was a problem hiding this comment.
It seems inevitable we'll need some concept of global state which can be accessed and updated from disparate components of the application. Redux is a good solution for this, but I find can be a pain point for new developer onboarding. If this were a path we'd go down, I'd want to make a conscious effort to keep it simple, ideally a single top-level reducer with a few obvious actions (maybe not even action creators or selectors).
There was a problem hiding this comment.
Why would Redux be a pain point for onboarding? Currently it's also not that evident where all the state is stored?
There was a problem hiding this comment.
Why would Redux be a pain point for onboarding?
Because many of the concepts are not self-evident:
aduth
force-pushed
the
add/selected-hover-state
branch
from
0177f1e
to
db21197
Compare
|
I rebased and refactored this pull request with consideration of #368, using global state to track hovered and selected block. This works quite well, even supporting tab-based focus navigation. The only issue I still need to work through is that after making a change to a text block and clicking out, it becomes focused again. It appears TinyMCE is emitting a |
|
Adding accessibility label because I needed to explicitly disable an accessibility lint rule to support blocks receiving focus while also containing block children (block in the latter case referring to its |
aduth
added
the
[Focus] Accessibility (a11y)
|
I'm going to merge this and create a bug issue tracking the unexpected TinyMCE change + focusout behavior. Edit: Bug tracked at #370 |
In this PR, I'm adding the "selected" and "hover" state for blocks.
For now, hovering a block shows the left border while selecting it shows the full border.
Typing the keyboard inside a block unselect it.