Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Re-rendering after props change adds another toolbar #147

Closed
stevenwinslowwhite opened this issue Jan 24, 2017 · 21 comments · Fixed by #177
Closed

Re-rendering after props change adds another toolbar #147

stevenwinslowwhite opened this issue Jan 24, 2017 · 21 comments · Fixed by #177

Comments

@stevenwinslowwhite
Copy link

stevenwinslowwhite commented Jan 24, 2017

When pasting elements into quill editor, all formatting is lost if I provide ReactQuill with a formats prop. If I remove the formats={formats}, then a copy paste of formatted text inserts properly (still formatted).

Below is a snippet of code:

const formats = [
    "list",
    "link",
    "bullet",
    "italic",
    "bold",
    "color",
    "font",
    "size",
    "image",
    { name: "h1", tag: "H1", prepare: "heading", type: "line" },
    { name: "h2", tag: "H2", prepare: "heading", type: "line" },
    { name: "h3", tag: "H3", prepare: "heading", type: "line" },
    { name: "p",  tag: "P",  prepare: "formatBlock", type: "line" },
];

    render() {
        return (
            <ReactQuill
                onChange={(content) => this._onEditorChange(content)}
                className={'QuillEditor'}
                value={this.props.content}
                formats={formats}>

                <div key="editor"
                     ref="editor" ></div>

                <ReactQuill.Toolbar
                    key="toolbar"
                    ref="toolbar"
                    items={toolbarItems}
                />
            </ReactQuill>
        );
    }

Is this expected? What can I do to debug the issue?

Versions: react-quill: 0.4.1, Quill: 1.2.0

@alexkrolick
Copy link
Collaborator

Try the version from master, Quill 1.x is out of date.

@stevenwinslowwhite
Copy link
Author

I'm a bit confused - you say that Quill 1.x is out of date, but I'm looking at https://www.npmjs.com/package/quill, and 1.2 is the latest version. If you're talking about react-quill, I see the latest version is 0.4.1, which is what I'm using.

Are you recommending not using the npm published version?

@alexkrolick
Copy link
Collaborator

Sorry, yes, the react-quill package on NPM is pre-1.0 and bundles a pre-1.0 version of Quill.

The Github version on master and the releases at https://github.com/alexkrolick/react-quill/releases use Quill 1.x and have a corresponding major version bump.

@stevenwinslowwhite
Copy link
Author

I'm looking into feasibility of pointing to github instead of npm in our build system. In the meantime, is there a reason that a more recent version of master isn't being pushed to npm?

@alexkrolick
Copy link
Collaborator

In package.json, you can point to a release tag:

"react-quill": "alexkrolick/react-quill#v2.0.2",
"react-quill": "zenoamaro/react-quill",

Release discussion for 1.0 #120

Relevant PR: #134

@stevenwinslowwhite
Copy link
Author

Any major gotcha's I should know about when upgrading to 2.0.1 from 0.4.1? I just did the package.json updates, and getting:

document is not defined

from line 2377 of quill.js:

var elem = document.createElement('div');

@alexkrolick
Copy link
Collaborator

alexkrolick commented Jan 25, 2017 via email

@stevenwinslowwhite
Copy link
Author

stevenwinslowwhite commented Jan 25, 2017

Yes - we do server side rendering. I've disabled that so now it only renders client side, and getting tons of issues w/ the toolbar. I've updated the component to use a module for the toolbar, but it is completely horked. Whenever the textarea is updated, componentDidUpdate is being called, which calls componentDidMount, which calls var editor = this.createEditor( this.getEditorElement(), this.getEditorConfig());, which eventually in quill.js creates a new toolbar. Therefore, for every character I type, I get a new toolbar. Am I somehow out of sync w/ proper changes? I'm now running with 1.2.0 Quill and 2.0.1 react-quill.

screen shot 2017-01-25 at 1 59 11 pm

@stevenwinslowwhite
Copy link
Author

Hey Alex - any update here? Are you fairly certain that the latest commit is stable, and doesn't have issues w/ the toolbar?

@alexkrolick
Copy link
Collaborator

alexkrolick commented Jan 30, 2017

Looking at https://github.com/alexkrolick/react-quill/blob/v2.0.2/src/component.js I'm seeing

screen shot 2017-01-30 at 2 01 09 pm

screen shot 2017-01-30 at 2 02 11 pm

Are you using a configuration like this: https://github.com/alexkrolick/react-quill#toolbar? It seems like you must be sending one of the "dirty" props in a parent component that would trigger a rerender of the quill container.

@stevenwinslowwhite
Copy link
Author

Wow - because I was inlining the module in the declaration, when component.js was doing equality for the dirty props, it was seeing this.props["modules"] as !== prevProps["modules"].

I think the last thing I'm looking at now is it doesn't appear that the newer code supports formats. Is format supposed to go through a module now? I don't see that on quill's docs, however I do see in component.js that setCustomFormats is commented out. New preferred way to set formats for > 1.0?

// this.setCustomFormats(editor); // deprecated in Quill v1.0

@stevenwinslowwhite
Copy link
Author

Meh - this appears to be some css that isn't being applied properly. I'm all set now - thanks for the help, closing out bug.

@zenoamaro
Copy link
Owner

This is an actual issue that needs to be dealt with.

Re-rendering Quill is causing toolbars to multiply exponentially.

We're most definitely doing something wrong when re-instantiating Quill.

@zenoamaro zenoamaro reopened this Mar 8, 2017
@alexkrolick
Copy link
Collaborator

I'm having difficulty reproducing this one.

In this example with the deprecated component ReactQuill.Toolbar as a child the Toolbar component actually seems to have no effect at all; it just renders the Quill defaults: http://codepen.io/alexkrolick/pen/WpogXy?editors=001

In this one with a custom toolbar ID provided to Quill via the modules prop, it works fine: http://codepen.io/alexkrolick/pen/gmroPj

@zenoamaro
Copy link
Owner

@alexkrolick, the issue is not specifically about toolbars, however the symptoms are the same.

Check this codepen here, and interact with the editor: http://codepen.io/anon/pen/KWqNbr

I am causing Quill to re-render each time by setting a bogus random class on ReactQuill – ideally nothing of the sort should happen.

@alexkrolick alexkrolick changed the title Copy/Pasting into react-quill not working as expected Re-rendering after props change adds another toolbar Mar 15, 2017
@alexkrolick
Copy link
Collaborator

The basic problem is that Quill creates uncontrolled DOM elements that don't get cleaned up after a remount/rerender. I can stop the additional of the toolbar by deleting the previous toolbar from the DOM in the unmount handler, but it causes the editor to be unfocused on the next tick. Working on a fix for that.

@alexkrolick
Copy link
Collaborator

Will have to pay attention to this change in the unmount/mount ordering coming in React 16: facebook/react#9214 (comment)

@zenoamaro
Copy link
Owner

@alexkrolick, good catch. I guess this is a Quill bug actually? So I'd be fine with applying hacks here until it's fixed.

@alexkrolick
Copy link
Collaborator

@alexkrolick
Copy link
Collaborator

Interestingly I can get a simple example working where React resets the DOM successfully after a props change:

http://codepen.io/alexkrolick/pen/yMEXPe

I wonder if the issue is the toolbar is being created outside of the DOM element created by React?

@alexkrolick
Copy link
Collaborator

I've updated the Codepen to isolate the problem: https://codepen.io/alexkrolick/pen/yMEXPe?editors=0010

The issue is very much the position of the toolbar in the DOM tree. It's essentially a sibling of the node created by the component, so it sits outside of the tree that React knows about and thus never gets cleaned up by the reconciliation algorithm.

https://facebook.github.io/react/docs/reconciliation.html

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants