INT-3226: Modernise the editor component to become a function component with hooks #577
Conversation
hamza0867
requested review from
spocke,
TheSpyder and
ltrouton
and removed request for
a team
ltrouton
requested review from
a team,
Skylite73,
shanmen-tiny,
lorenzo-pomili,
ztomaszyk,
michalnieruchalski-tiugo and
tiny-james
and removed request for
a team
| elementRef.current = el; | ||
| if (typeof ref === 'function') { | ||
| ref(el); | ||
| } else if (ref) { | ||
| ref.current = el; | ||
| } |
There was a problem hiding this comment.
Reduce the duplicated ref handling logic?
There was a problem hiding this comment.
What do you mean ? What is duplicated ? You mean refactor out this function
(el) => {
elementRef.current = el;
if (typeof ref === 'function') {
ref(el);
} else if (ref) {
ref.current = el;
}And use it for both Iframe and Inline ?
There was a problem hiding this comment.
Yes. They look the same to me
| } else { | ||
| // give up, at this point it seems that more polling is unlikely to help |
There was a problem hiding this comment.
I assume you did a find and replace for this and accidentally removed it from this comment too. The new comment doesn't make any sense so I'd suggest tweaking it or putting this back...
There was a problem hiding this comment.
Indeed 🤦♂️. I used a regex that was supposed to match literally "this." but apparently it matched this*
| editorRef.current.off(beforeInputEvent(), handleBeforeInput); | ||
| editorRef.current.off('keydown', handleBeforeInputSpecial); | ||
| editorRef.current.off('keyup', handleEditorChangeSpecial); | ||
| editorRef.current.off('NewBlock', handleEditorChange); |
There was a problem hiding this comment.
I suspect these off statements will not work because the references to the functions will have changed before off is run. For example handleEditorChange is dependent on onEditorChange so if the user deregisters that prop it will update the function handleEditorChange points to before it tries to deregister the event handlers.
Maybe this is possible with a cleanup on the callback constructors? Be careful because leaving these registered will have very strange results.
| } | ||
| } | ||
| } | ||
| }; | ||
| }, [ value, onEditorChange, rollback, rollbackChange ]); |
There was a problem hiding this comment.
There is no way you want this to regenerate on changing value... The only way this will work is if it references value though a ref. I suspect you'll have to reference everything through refs because this is a TinyMCE event handler and unless you are constantly deregistering the old version and re-registering the new version this is not going to work.
There was a problem hiding this comment.
(yes it will be a kind of hell getting this to work - that is why I never moved off the class based implementation)
There was a problem hiding this comment.
I am pretty sure this is incorrect as currently written because of the way the tinymce event handlers are written to update on many props. I think you're going to have to make the event handlers pretty much static and only interacting through refs so they don't have to update. This is a challenging change and I'm fairly impressed with with I see so far.
Hey @tiny-james, thanks for reviewing this! Would you be able to suggest some scenarios that could capture these concerns as test cases? I hadn't considered these as issues either, so validating them against the current implementation and these changes would deepen our understanding of the integration and give us more confidence in the refactor. Sorry to jump in, @hamza0867! Just saw an opportunity to strengthen the project's robustness 😄 |
|
@kimwoodfield No worries. What james is talking about there is registering many listeners for example or having weird side effects. As it is written, if you open the storybook demo for Controlled component and try to type a letter, then when you investigate you will find that the After further investigation I agree with @tiny-james that the current implementation is not correct. What I think is we need to rethink how to write the component as if we were to write from scratch so we can split up the effects correctly and clean them correctly as well. |
|
@hamza0867 can we close this PR if you are not planning to continue working any time soon? I can create a ticket for future investigation |
|
@tiny-ben-tran Yes I am closing this PR, please create a ticket for future investigation. |
No description provided.