-
Notifications
You must be signed in to change notification settings - Fork 41
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
Add value prop for convenience #16
base: main
Are you sure you want to change the base?
Conversation
Is a numeric value instead of a one-element array
oh hey @zqianem ! This is kind of what I had running originally as a workaround (as I described in #12) but I was unable to achieve what you have here (I didn't think about two-way binding the values to each other)... I haven't had time to check it out and play with it yet but I can see the logic and it seems sound. I just worry about recursion or looping, or passing both props simultaneously. I'll get around to testing it thoroughly and determine the impact it has in due course... but for now thanks a lot for the contribution !! 🙇 |
export let vertical = false; | ||
export let float = false; | ||
export let hover = true; | ||
|
||
// keep value and values in sync with each other | ||
const updateValues = () => { if (value !== values[0]) values = [value] }; | ||
const updateValue = () => { if (value !== values[0]) value = values[0] }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ran into a situation where I wanted to have value
to be a continuous value while having relatively large discrete step sizes for the RangeSlider, so I changed the update check in my local patch from value !== values[0]
to alignValueToStep(value) !== values[0]
. That way, I could update value
elsewhere in my app without it being aligned to the RangeSlider step.
Would you want to have this behavior by default? I'm okay either way as I can maintain my patch if not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you do a REPL with and without the behavior as an example, @zqianem ?
I had this patch checked out locally for testing the other day, but didn't get review completed, I don't 100% follow this comment/change though... are you saying that because this change reacts to value
and then eventually goes through alignValueToStep()
that eventually propogates back and changes the input value
to align ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://svelte.dev/repl/4fe353de6b1d4248bce140c61903a367?version=3.31.2
Here's a REPL showing the behaviour before your PR, with the values[]
property instead ... I think this is the desired behaviour, it does feel a bit weird when the value can change from elsewhere, but equally it would be very strange to step a slider but allow the bound value to not adhere to the rules of it, don't you think?
I'm willing to hear opinions on this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's a REPL comparing the implementations: https://svelte.dev/repl/061bbed31e2c4529a1a267aa63d7e5dd?version=3.31.2
I think the middle example (New Behavior (value prop)
) is the ideal, where the slider with the step size of 1 changes the value by ones, while the slider with the step size of 10 can only change the value by tens. The first and last examples are a bit odd in that the opposite behavior occurs; not sure how to best address this as I think we want the value
and values
props to behave as similarly as possible.
More concretely, here is the project where I was using this behavior: https://5ff673a5b316db24c45db7c2--loving-morse-6b791c.netlify.app/. The globe can be rotated by either dragging its surface or opening the "Perspective" menu and using the sliders. Before the alignValueToStep(value) !== values[0
patch, the slider would cause the dragging of the globe to be snapped to the step size of the slider, but afterwards, it is free to move smoothly. (Here is the globe before the patch: https://5ff0b9f53aa2402db335c823--loving-morse-6b791c.netlify.app/; notice how the dragging of the globe is snapped and not smooth.)
Is a numeric value instead of a one-element array.
This is a bit of hack, admittedly. Addresses #12. Not sure how you wanted to update the docs for this.