zenoamaro / react-quill

A Quill component for React.
https://zenoamaro.github.io/react-quill
MIT License
6.77k stars 923 forks source link

Dropdown default values not working with custom JSX toolbar #175

Open kamas06 opened 7 years ago

kamas06 commented 7 years ago

Expected: When you select a text with "normal" font size, the dropdown should reflect that selection.

Actual: Dropdown shows "large" or "small" but never goes back to "normal". It looks to be only happening with manually created HTML toolbar and only with React. Tired modding official Quill codepen to manual toolbar and it works there with plain HTML.

image image

I assume this is because of the way React handles select tags - https://facebook.github.io/react/docs/forms.html#the-select-tag

I have tried using defaultValue on select etc. but no luck.

http://codepen.io/kamas06/pen/EWLpKm

Quill version:

alexkrolick commented 7 years ago

~I'm seeing this for your Codepen on macOS Sierra, Google Chrome. What are you using?~

EDIT: Ok I see there is an issue here. Dropping a link to the relevant Quill modules for investigation: https://github.com/quilljs/quill/blob/develop/modules/toolbar.js

alexkrolick commented 7 years ago

Here's a side-by-side of the different toolbar options: https://codepen.io/alexkrolick/pen/BWVNYx?editors=0010

The JSX custom toolbar option is definitely doing something a little differently than the uncontrolled way - note extra bottom border between the elements.

kamas06 commented 7 years ago

@alexkrolick Nice side-by-side, shows the problem really well. I got hit by #147 too at some point but moved away to JSX toolbar which sorted it. Had to move anyway to get more control over it as I am placing some custom markers/icons inside it.

alexkrolick commented 7 years ago

Note that is possible to have some control over the appearance of the icons Quill creates with the toolbar prop. Use CSS to target ql-<format>: https://codepen.io/alexkrolick/pen/qryGQd

Will continue looking into the issue.

alexkrolick commented 7 years ago

Some additional debugging notes:

This line requires one of the <option> tags to have the selected attribute: https://github.com/quilljs/quill/blob/develop/ui/picker.js#L46

If it is missing, the default item will not get the ql-selected class here: https://github.com/quilljs/quill/blob/develop/ui/picker.js#L73

And finally, the defaultItem will not exist here: https://github.com/quilljs/quill/blob/develop/ui/icon-picker.js#L11-L18

screen shot 2017-07-08 at 11 19 43 pm

Demo: https://codepen.io/alexkrolick/pen/qjMrEN?editors=0010

Note that setting THEME to false at the top actually works better when it comes to dropdowns than the themes that extend base (ie Snow and Bubble) because it only uses HTML selects, rather than the "fake" select from icon-picker.js

So... it seems the main thing to do is figure out a way to 1) get React to stop swallowing the selected attribute on options in selects or 2) add an alternate identifier for Quill to use to determine the selected state, that still emits the right change events

NOTE: React doesn't allow setting selected manually for each tag (instead it is set by the select's value): https://stackoverflow.com/questions/21733847/react-jsx-selecting-selected-on-selected-select-option#comment33279210_21736116

jxmai commented 7 years ago

So... it seems the main thing to do is figure out a way to 1) get React to stop swallowing the selected attribute on options in selects or 2) add an alternate identifier for Quill to use to determine the selected state, that still emits the right change events

@alexkrolick I am coming from another project (https://www.primefaces.org/) which is heavily used by enterprise since your change seems broke our code (https://github.com/primefaces/primefaces/issues/2802), and I have raised another ticket in quill https://github.com/quilljs/quill/issues/1762

alexkrolick commented 6 years ago

💯 I think this can be fixed by adding some hints for React's event system:

<select defaultValue={""} onChange={e => e.persist()}>

https://codepen.io/alexkrolick/pen/gmroPj?editors=0010

coxandrew commented 6 years ago

Hi @alexkrolick - I'm trying to wrap Quill in a React component for my own custom implementation and I cannot cannot figure out why react-quill is working with a defaultValue, but mine is not (the onChange doesn't appear to be necessary in your example above).

Here's a streamlined example demonstrating the bug: https://codepen.io/coxandrew/pen/xjxXmy?editors=1011

I've tried with and without jsx toolbar components and about every combination of selected or value={""} within the option tag and cannot get it to work with my simple example.

I have another CodePen that uses a forked version of Quill with this attempted fix, but that doesn't work with my React version either: https://codepen.io/coxandrew/pen/YLzvwJ?editors=0011

Any ideas on what might be different about react-quill's implementation?

alexkrolick commented 6 years ago

@coxandrew seems to work when I upgrade your Pen to React 16: https://codepen.io/alexkrolick/pen/KRKYLw?editors=1011

coxandrew commented 6 years ago

@alexkrolick Aha! Thanks for the quick response! We may not be able to upgrade to React 16 immediately, but at least we have a path forward.