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

GuidedValueSlier: Click on value to type into text field #12263

Merged
merged 1 commit into from
Dec 31, 2024

Conversation

DonLakeFlyer
Copy link
Contributor

Guided.mov

@DonLakeFlyer
Copy link
Contributor Author

@DieBorr Can you look through this?

@DieBorr
Copy link
Contributor

DieBorr commented Dec 30, 2024

Ok, I will check it right now.

@DieBorr
Copy link
Contributor

DieBorr commented Dec 30, 2024

Okay, the textField label itself is very well integrated, however I can see a glitch while moving the slider with or without the textField enabled, here is a video:

simplescreenrecorder-2024-12-30_11.55.09.mp4

The problem is in :
return Math.min(Math.max(value.toFixed(decimalPlaces), _sliderMinVal), _sliderMaxVal).toFixed(decimalPlaces)
at _clampedSliderValue function, math.min returns an int when you send a float number with non significant decimal numbers so I suggest to replace that line with:

Math.min(Math.max(value.toFixed(decimalPlaces), _sliderMinVal), _sliderMaxVal).toFixed(decimalPlaces)

I could make a pull request but with just this little fix it does not make sense
Thank you !

@DieBorr
Copy link
Contributor

DieBorr commented Dec 30, 2024

Finally I have made a pull request because there was some more things that I think that will improve the user experience

@DonLakeFlyer
Copy link
Contributor Author

Yup, that is screwed up. Fixed.

@DonLakeFlyer DonLakeFlyer merged commit 0edc782 into master Dec 31, 2024
8 checks passed
@DonLakeFlyer DonLakeFlyer deleted the GuidedValueType branch December 31, 2024 01:32
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.

2 participants