zillow / react-slider

Accessible, CSS agnostic, slider component for React.
https://zillow.github.io/react-slider
MIT License
892 stars 231 forks source link

Fix "This synthetic event is reused for performance reasons" warning #73

Closed moroshko closed 8 years ago

moroshko commented 8 years ago

When dragging the slider, React 15.0 complains:

Warning: This synthetic event is reused for performance reasons. If you're seeing this, you're adding a new property in the synthetic event object. The property is never released. See https://fb.me/react-event-pooling for more information.

This PR fixes this warning.

xiaolin commented 8 years ago

:+1:

crucialfelix commented 8 years ago

That fixes the warning, but I think there's a little design flaw here.

If you just want to disable text selection then add some inline styling to it

.noselect {
  -webkit-touch-callout: none; /* iOS Safari */
  -webkit-user-select: none;   /* Chrome/Safari/Opera */
  -khtml-user-select: none;    /* Konqueror */
  -moz-user-select: none;      /* Firefox */
  -ms-user-select: none;       /* Internet Explorer/Edge */
  user-select: none;           /* Non-prefixed version, currently
                                  not supported by any browser */
}
moroshko commented 8 years ago

@crucialfelix This CSS wouldn't fix the warning, right?

crucialfelix commented 8 years ago

It would mean that you wouldn't have to molest the event in order to prevent text selection. That is why that function is in there (the one triggering the warning). It is altering the event so that when it bubbles up it doesn't result in selecting the text.

It's the wrong way to solve the problem. Better to just tell the browser using CSS that that is not a text area to be selected.

The warning is there for a reason, and the SyntheticEvent pool is there for performance reasons. A drag event is going to generate a lot of them, and that's going to impact performance.

moroshko commented 8 years ago

@crucialfelix Looks like you understand better than me what's going on here. Mind submitting a PR that properly solves the issue?

crucialfelix commented 8 years ago

Possibly, but I'm a bit overloaded right now. The css I put in above should prevent user text selection. with that, you no longer need to worry about the event.

On Mon, Apr 18, 2016 at 1:25 PM Misha Moroshko notifications@github.com wrote:

@crucialfelix https://github.com/crucialfelix Looks like you understand better than me what's going on here. Mind submitting a PR that properly solves the issue?

— You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub https://github.com/mpowaga/react-slider/pull/73#issuecomment-211337174

https://twitter.com/crucialfelix https://medium.com/@crucialfelix http://www.mattermind.com/

jamesbrauman commented 8 years ago

@moroshko Any plans to update this pull request to use CSS as per the suggestion by @crucialfelix ?

moroshko commented 8 years ago

@jamesbrauman I don't have time at the moment. Happy for you to take over, if you keen to contribute.

jamesbrauman commented 8 years ago

@moroshko I created a pull request that solves this issue, without introducing any breaking chances by requiring users to set the user-select CSS property. https://github.com/mpowaga/react-slider/pull/80

AndersDJohnson commented 8 years ago

76

jamesbrauman commented 8 years ago

@moroshko A fix for this issue has been deployed and is available in release 0.7.0.