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

Use editor-change event to call onChange and onChangeSelection properties #382

Merged

Conversation

bakkerjoeri
Copy link
Contributor

@bakkerjoeri bakkerjoeri commented Jul 4, 2018

Closes #381

update-selection-immediately

editor-change's selection change seems to more completely cover the cases in which one expects onChangeSelection to be called.

Another idea was exposing onEditorChange as a prop, and although this might provide better backwards compatibility, it also seems more confusing.

I'm curious to hear whether my proposed approach seems valid, or if you perhaps expect it to break other parts of the wrapper.

@bakkerjoeri bakkerjoeri changed the title Use editor-change to call onChange and onChangeSelection properties Use editor-change event to call onChange and onChangeSelection properties Jul 4, 2018
@alexkrolick
Copy link
Collaborator

editor-change
Emitted when either text-change or selection-change would be emitted, even when the source is "silent". The first parameter is the event name, either text-change or selection-change, followed by the arguments normally passed to those respective handlers.

My main concern would be more false-positives for selection change events when interacting with the toolbar - for example #367

@alexkrolick
Copy link
Collaborator

Does this fix #394?

@bakkerjoeri
Copy link
Contributor Author

@alexkrolick

My main concern would be more false-positives for selection change events when interacting with the toolbar

I totally get that; the change I propose here feels like something that could have a lot of unforeseen side effects. All I can say is that for a while I've been using a fork with the change in this PR for a while now and things still seem to work as they should, including toolbar interactions.

@bakkerjoeri
Copy link
Contributor Author

bakkerjoeri commented Sep 10, 2018

@alexkrolick

Does this fix #394?

I'm not sure it's related. It might have something to do with scrollingContainer though (see #239).

@parhamcurtis
Copy link

I have used this change and I can confirm this fixes the issue. I have not seen any downside to this yet! Please accept this!

@alexkrolick alexkrolick merged commit eb45701 into zenoamaro:master Oct 27, 2018
@alexkrolick
Copy link
Collaborator

alexkrolick commented Oct 27, 2018

Published at 1.3.2

Thanks for everyone's input!

@bakkerjoeri bakkerjoeri deleted the bugfix/listen-to-editor-change branch October 28, 2018 11:33
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 this pull request may close these issues.

3 participants