Editor Chrome: Adding the editor mode switcher #339
Conversation
|
🎉 Solid work! I'm setting up VVV and getting my env up so I can contribute to these branches. There are some little pixel nudges I'd like to do here and there, but nothing you should necessarily worry about. If you'd like to get this in fast, 👍 from me! This might sound controversial — do we want to show those notices, "get version" and "thanks for creating with WordPress in the editor screen? I say we remove it all for now, and add it back only after presented with good arguments for why they need to be everywhere. |
editor/editor/layout.js
Outdated
| } | ||
|
|
||
| componentDidMount() { | ||
| this.setState( { |
There was a problem hiding this comment.
At the very least we should try to do this in componentWillMount or the constructor to avoid an immediate re-render upon mount from setState. Someone shared this with me yesterday which I found quite informative about lifecycle methods and interactions with state:
https://gist.github.com/bvaughn/923dffb2cd9504ee440791fade8db5f9
But more importantly there might be some need to rethink how we're controlling or at least naming the initial content being passed in. Perhaps naming the prop as defaultContent (like defaultValue of base inputs) with a preference toward the state value in the render, or initialContent (initialValue) in defining the initial state.
editor/editor/layout.js
Outdated
| <ModeSwitcher mode={ mode } onSwitch={ this.switchMode } /> | ||
| </div> | ||
| <div className="editor__body"> | ||
| { mode === 'text' && <EditorText html={ html } onChange={ this.changeHtml } /> } |
There was a problem hiding this comment.
Minor: Possible to consolidate similar logic, though dunno how much this helps readability:
createChangeHandler( type ) {
return ( value ) => this.setState( { [ type ]: value } );
}
// ...
render() {
<EditorText html={ html } onChange={ this.createChangeHandler( 'html' ) } />
}
editor/editor/mode-switcher.js
Outdated
| } | ||
|
|
||
| toggle( event ) { | ||
| event.preventDefault(); |
There was a problem hiding this comment.
If you assign type="button" to the button element you can avoid the need for preventDefault (only needed because default type of button is "submit").
There was a problem hiding this comment.
If you assign
type="button"to thebuttonelement you can avoid the need forpreventDefault(only needed because defaulttypeofbuttonis"submit").
I guess it makes no difference at the moment since the button lives outside the page form, but having not assigned a type to the button may come to bite us in the future.
| <button | ||
| className="editor-mode-switcher__toggle" | ||
| onClick={ this.toggle } | ||
| aria-label="Switch the editor mode" |
There was a problem hiding this comment.
We should probably start thinking about localization sooner than later. I might focus on this today (for a separate pull request).
editor/editor/mode/visual.js
Outdated
|
|
||
| return ( | ||
| <div className="editor-mode-visual"> | ||
| <div contentEditable> |
There was a problem hiding this comment.
contentEditable was removed in #330 and we'll probably want to do the same here.
facbb97 to
c820350
Compare
|
@jasmussen I had to rebase this branch to resolve the conflicts, you may have to reset your local branch. |
editor/editor/mode/text.js
Outdated
| @@ -0,0 +1,9 @@ | |||
| function Text( { html, onChange } ) { | |||
| return ( | |||
| <div className="editor-mode-text"> | |||
There was a problem hiding this comment.
Not much, but it eases styling and I'm certain we'll have other elements added soon.
- Adding the `onChange` content handler - v1 of the editor header - Adding a v1 the text editor
… the constructor - Adding type="button" to buttons
faeac4a to
9f3e225
Compare
Several things in this PR:
onChangecontent handler and updating the content on the Editor's state