Fix CodeEditor not loading when WordPress is installed in a subfolder #6777
Conversation
gziolo
added
[Feature] UI Components
noisysocks
left a comment
noisysocks
left a comment
There was a problem hiding this comment.
Thanks so much for looking into this! ❤️
I think that a simpler way to fix #6737 is to change components/code-editor/editor.js:107 to use defaultValue instead of value. We then don't need to worry about changing onBlur to onChange.
components/code-editor/editor.js
Outdated
|
|
||
| if ( ! this.props.focus && this.editor.hasFocus() ) { | ||
| document.activeElement.blur(); | ||
| document.activeElement.change(); |
There was a problem hiding this comment.
HTMLElement doesn't have a change() method, so this would always result in an undefined is not a function exception.
https://developer.mozilla.org/en-US/docs/Web/API/HTMLElement
components/code-editor/editor.js
Outdated
|
|
||
| render() { | ||
| return <textarea ref={ ( ref ) => ( this.textarea = ref ) } value={ this.props.value } />; | ||
| return <textarea ref={ ( ref ) => ( this.textarea = ref ) } value={ this.props.value } onChange={ this.props.onChange } />; |
There was a problem hiding this comment.
We're calling this.props.onChange() both here in the text field's onChange handler and also in our this.editor.on( 'change', ... ) handler. I fear that this means we're calling this.props.onChange() twice on every keystroke.
I think that a better approach would be to make this text area an uncontrolled component and then let CodeMirror be solely responsible for handling user input and dispatching its change event.
return <textarea ref={ ( ref ) => ( this.textarea = ref ) } defaultValue={ this.props.value } />;
noisysocks
changed the title
|
Thanks for the detailed review and info. Initially thought to use defaultValue but don't know why took that adventurous turn! Have updated & tested the PR. |
|
I tested this locally and it looks good to go 👍 There's one failing snapshot test that we'll need to fix before merging. This should be just a matter of updating the snapshot tests using this command: $(npm bin)/jest --config test/unit/jest.config.json --updateSnapshot |
|
@noisysocks snapshots updated |
|
|
||
| const script = document.createElement( 'script' ); | ||
| script.src = `/wp-admin/load-scripts.php?load=${ handles.join( ',' ) }`; | ||
| script.src = `${ wpApiSettings.schema.url }/wp-admin/load-scripts.php?load=${ handles.join( ',' ) }`; |
There was a problem hiding this comment.
It's probably not the only place where we use this, but we should move away from using globals. I'm also wondering if the CodeEditor belongs in the components module since the components module is supposed to be for generic components (without WP dependency?).
Anyway, this is not urgent and will have to be figured out when we move components to an npm dependency anyway. And this is not specific to this component.
5 participants
Description
How has this been tested?
Manual
Types of changes
Bug fixes
Checklist: