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

Bug Fix: Always fire rangeValueChanged event when thumb is dragged #6680

Merged
merged 1 commit into from
Nov 13, 2023

Conversation

JamesHollyer
Copy link
Contributor

@JamesHollyer JamesHollyer commented Nov 9, 2023

Motivation for features / changes

The rangeInputComponent was not triggering the rangeValueChanged event when dragging the slider thumb. This fixes that bug.

Technical description of changes

The slider changes the value of lowerValue and upperValue while the thumb is being dragged. When dragging is stopped the component used to ensure the value sent in the event was different from the previous values by comparing it to the lowerValue and upperValue. This check always failed as the component now keeps these values up to date. It should be noted that if these values are not kept up to date the input fields do not update while dragging.

To solve I simply call the event every time without doing the check. If the value has not changed we update the state to the same value. This does cause an unnecessary repaint but that seems reasonable for when I user clicks the thumb and does not move it.

I decided I could simply ignore the value in the event as the lowerValue and upperValue properties are already updated. This simplifies the code by having one place to call no matter which thumb is dragged.

Copy link
Contributor

@rileyajones rileyajones left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

I thought you were planning on deleting this component altogether?

@JamesHollyer
Copy link
Contributor Author

LGTM

I thought you were planning on deleting this component altogether?

Yea that was a thought. It turned out to be more work than I wanted to do for this migration. The component is much more simple now though.

@JamesHollyer JamesHollyer merged commit dc05adf into tensorflow:master Nov 13, 2023
13 checks passed
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