Closed rijk closed 5 years ago
Holy moly, you are correct. Perhaps the easiest and most dynamic (and riskiest?) solution would be to just spread all unknown props onto the root Slider
element.
You should be able to solve it using the wrapper idea too!
Do you mean onto the <div>
in the Slider
component? Yep I guess that would work! I would definitely prefer that over a wrapper div by the way. Alternatively you can also add the onMouseLeave as an optional prop on Slider
. Although — hold on, shouldn't the onMouseLeave be on the RangeControlOverlay
instead of the div
? In that case, I would definitely go with an optional prop so that you can make sure it's put on the right element internally.
Good questions. What are the benefits of spreading them onto RangeControlOverlay
? If we do that, we need to educate people on it, and I have this far regarded it as internal sorcery.
If we create a new explicit onMouseLeave
prop, wouldn't we need to create props for all events?
Hm, I don't know, I think onMouseLeave
is the most needed because you need it to cancel intent. An alternative fix for this (because I see your point about the other events) could be to implement onMouseLeave
internally, but make it call onIntent( null )
.
Now that I think of it this might actually be the best fix, as the RangeControlOverlay
calls onIntent
while hovering as well. So it would make sense for it to 'clean up' on mouse out.
Hmmmmm very interesting
If we look at how the change events are handled, we would introduce onIntentStart
and onIntentEnd
, which would be more explicit than checking if value === null
. Plus, it would not be a breaking change.
What do you think?
That would definitely work! 👍
Have a look at https://github.com/alexanderwallin/react-player-controls/pull/48 and see if it looks reasonable to you!
Yup, that looks really good! Although you are trying to keep the built-in functionality to a minimum, I think it's a worthwhile addition.
I've raised a question in the PR, and would appreciate your feedback, @rijk!
The example in the readme (specifically the one for "Progress bar that shows the target time on hover"), uses the
onMouseLeave
property onSlider
. However, this does not seem to be implemented (yet).Are there plans to add this? (would be great) Or should I add a wrapper div to implement it myself?