NEW HtmlEditorField component for react rich text #628
Conversation
|
The omnipotent observer may note that it seems like this may close #623 - however I don't believe it does by the manner in which it invokes TinyMCE... this PR still relies on Entwine due to reference issues encountered when attempting to use the TinyMCE component. The issue (#623) should remain open, but evolve to find a more wholesome solution to the problem. |
client/src/components/Form/Form.js
Outdated
| @@ -102,10 +104,12 @@ Form.propTypes = { | |||
| value: PropTypes.any, | |||
| type: PropTypes.string, | |||
| })), | |||
| formTag: PropTypes.node, | |||
There was a problem hiding this comment.
node is pretty broad. Is that deliberate? I guess it means you could inject a component right? If that is the intention then all good
There was a problem hiding this comment.
Yeah, I'm not 100% if it's the correct thing, it just seemed the closest thing to me. The idea is to be able to specify an HTML tag, or supply a custom component, yes :)
There was a problem hiding this comment.
I would recommend using formTag: PropTypes.oneOfType([PropTypes.func, PropTypes.string]), nowadays...
using PropTypes.node normally denotes that you'll output it in the render straight away: <div>{this.props.formStuff}</div>, which is perfect for children
There was a problem hiding this comment.
Neat thanks, I'll update :)
There was a problem hiding this comment.
Minor suggestion around adding doc blocks. I haven't tested the change but it looks good to me. Higher level I'd say that the changes in this PR could actually be two different PRs, one for making the form tag configurable and one for adding the TinyMCE React support.
It'd be good to get someone else from the @silverstripe/core-team to look over this before it gets merged.
| return <Script url={this.props.data.editorjs} onLoad={this.handleReady} />; | ||
| } | ||
|
|
||
| renderRichTextEditor() { |
There was a problem hiding this comment.
These two methods could probably use a minimal doc block
|
Also needs a rebase =) |
| } | ||
|
|
||
| renderRichTextEditor() { | ||
| const config = JSON.parse(this.props.data.config['data-config']); |
There was a problem hiding this comment.
this data.config['data-config'] is verbosely repetitive, what's the reason for the extra 'data-config' layer, and could it be moved up?
There was a problem hiding this comment.
This is the data as it's exported from HTMLEditorConfig(.php) - it consists of the config, and the editor reference (c.f. window.ss.editorWrappers), both of which are required to load the editor, and are applied directly to the element as data-attributes.
There was a problem hiding this comment.
As per silverstripe/silverstripe-framework#8359 (comment) I've renamed the prop to data.attributes
| } | ||
|
|
||
| renderDependencyScript() { | ||
| return <Script url={this.props.data.editorjs} onLoad={this.handleReady} />; |
There was a problem hiding this comment.
this is interesting, because when I had a go at implementing this, I ran into issues of the script loading multiple times if I had multiple instances of this rendered and on a slow connection.
Have you tested this out? Do you know if load-script manages that?
There was a problem hiding this comment.
I will investigate.
There was a problem hiding this comment.
No, it is safe :)
I tested in browser and it seemed fine.
Investigation into why revealed:
https://github.com/blueberryapps/react-load-script/blob/master/src/index.jsx#L46
| getInputProps() { | ||
| return { | ||
| ...super.getInputProps(), | ||
| ...this.props.data.config, |
There was a problem hiding this comment.
what kind of config props are spread here?
this adds a bit of ambiguity of what is expected to happen, could HtmlEditorField contain these props instead of getting them from the data.config prop?
There was a problem hiding this comment.
data-config, data-editor - both of which are required for TinyMCE to bootstrap.
This field is subclassing TextField component, the getInputProps of which essentially are the HTML node properties (id, className, etc.), so these data attributes fit nicely in this context.
There was a problem hiding this comment.
as above, data.config is now data.attributes - along with added docblocks, I hope confusing is reduced to any new reader :)
NightJar
force-pushed
the
pulls/1/ReactRichtext
branch
2 times, most recently
from
0a95114 to
0a3302c
Compare
|
D: I don't understand why these Behat tests are failing :/ |
|
The latest 1 branch breaks Behat tests, it's not just this PR: https://travis-ci.org/silverstripe/silverstripe-admin/builds/424174909 |
client/src/legacy/HtmlEditorField.js
Outdated
| @@ -298,14 +298,22 @@ jQuery.entwine('ss', function($) { | |||
| this._super(); | |||
| }, | |||
|
|
|||
| onadd: function() { | |||
| this.onmatch(); | |||
There was a problem hiding this comment.
This change is throwing errors when I try and add new content blocks now (testing silverstripe/silverstripe-elemental#372)
There was a problem hiding this comment.
Hmm, I only added it as a backwards compatibility thing because I wasn't sure about the differences between onadd and onmatch. My impression is that the former was deprecated for the latter, but there may be subtle differences. I'll dig in to investigate my suspicions... ideally we can just stick with the original change which was simply renaming onadd to onmatch (and no BC alias).
There was a problem hiding this comment.
No, they're intended for different proposes, onadd is specific to creating a dom element into that selector while onmatch covers a few more scenarios including onadd's
Either or can be used for most scenarios, but just don't call one with the other (let entwine decide that)
There was a problem hiding this comment.
OK, the differences are subtle and not very impacting. Simply exchanging onadd for onmatch should be safe IMO.
onadd: fires when an element is added (e.g.node.appendChild(newEl)) to the document by entwine.onmatch: fires whenever the dom is updated (e.g.el.className = 'omg'), by any method.
The latter is a superset encompassing the former (DOM is updated to add things), but has the bonus of being able to init things added by other systems (e.g. React).
As a side note, I have no idea why onadd can't reference onmatch in the same way it could access other entwine applied (magic) methods (such as setEditor). Oh well.
There was a problem hiding this comment.
oh, thanks for the response @flamerohr - I didn't see it before writing the above^
NightJar
force-pushed
the
pulls/1/ReactRichtext
branch
from
0a3302c to
de4ce2d
Compare
|
Behat is green again in 1 - rebase to take advantage of this |
|
Behat failures seem legitimate. 1 branch is passing, I've re-run this PR's builds a few times and they're still failing. |
|
Hmm, must be some phantom (read: hard to recreate) JS error occurring somehow. I can't reproduce in my environment as it stands, I'll have to take a closer look in the morning. |
|
I've recreated the test failures locally on the 1 branch, follow the Travis build steps |
|
Ok I found the bug - TinyMCE isn't reinitialising after publishing a page (window reloads with PJAX). This is probably because of the entwine match changes in this PR. Demo: https://screencast-o-matic.com/watch/cFQ6IIqEAl I realise that putting it back to |
NightJar
force-pushed
the
pulls/1/ReactRichtext
branch
2 times, most recently
from
64ed449 to
875b155
Compare
|
YUSSSSS 💚 |
|
😢 I don't understand why the merge check is 🔁 💛
|
This allows one to use a Rich Text field from within a react form, where as previously it was only possible from server side PHP rendering templates, and Entwine. The downside to this is that although `@tinymce/tinymce-react` is the official package for this, it is in turn only a very loose wrapper around the window.TinyMCE global. It also posed problems with the configuration generated by the PHP config class, and altering that would destroy backwards compatiblity - which for now is critical for adoption. So while there is now a React component for rendering TinyMCE, it is only a bare bone component that still relies on Entwine to boot the actual editor via the `data-config` attribute on the React output `textarea` element.
NightJar
force-pushed
the
pulls/1/ReactRichtext
branch
from
875b155 to
2394194
Compare
This allows one to use a Rich Text field from within a react form, where
as previously it was only possible from server side PHP rendering
templates, and Entwine.
The downside to this is that although
@tinymce/tinymce-reactis theofficial package for this, it is in turn only a very loose wrapper
around the window.TinyMCE global. It also posed problems with the
configuration generated by the PHP config class, and altering that would
destroy backwards compatiblity - which for now is critical for adoption.
So while there is now a React component for rendering TinyMCE, it is
only a bare bone component that still relies on Entwine to boot the
actual editor via the
data-configattribute on the React outputtextareaelement.In order to facilitate what is essentially inserting a form into a form (in terms of HTML semantics), it was necessary to alter
components/Form/Form.jsto render with a particular tag (as opposed to<form>always).Requires silverstripe/silverstripe-framework#8359
As part of work for silverstripe/silverstripe-elemental#327