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

Fix string->number conversion in 3D Tiles inspector #5355

Merged
merged 2 commits into from
May 24, 2017
Merged

Conversation

lilleyse
Copy link
Contributor

Values were being sent as strings rather than numbers, causing the sliders not to work.

@hpinkos could you look at this? I'm not sure if this is the correct thing to do.

@hpinkos
Copy link
Contributor

hpinkos commented May 24, 2017

Thanks @lilleyse! This looks like the right thing to do

@mramato
Copy link
Contributor

mramato commented May 24, 2017

Shouldn't this use Number(value) instead of parsefloat? Number makes sure the number is really a number, parseFloat ignores invalid data (for example "123qwe" will return 123 with parseFloat).

@hpinkos
Copy link
Contributor

hpinkos commented May 24, 2017

@mramato Yeah, you're right. I didn't think it would make a difference because it will always be a number value coming from the slider. I forgot that you can add invalid characters into the input textbox though.

@lilleyse make sure to add an isNaN check after casting the value to a number. I'm sure we only want to apply it to the tileset if it's a valid number.

@lilleyse
Copy link
Contributor Author

Updated.

Is there a way to tell the slider to always return a number? That would also fix this.

@mramato
Copy link
Contributor

mramato commented May 24, 2017

There is not, but even if there was you need to handle the case where a user enters a value manually in the text box above the slider.

@mramato
Copy link
Contributor

mramato commented May 24, 2017

Thanks @lilleyse.

@mramato mramato merged commit 557c157 into 3d-tiles May 24, 2017
@mramato mramato deleted the number-fix branch May 24, 2017 18:50
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.

3 participants