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

Make number slider more robust against weird input values #4758

Merged
merged 4 commits into from
Aug 10, 2020

Conversation

philippotto
Copy link
Member

@philippotto philippotto commented Aug 7, 2020

This is a workaround for layer opacity values which become undefined in some circumstances. The actual cause for this is unknown to me, but making the number slider more robust should be an okay way to tackle this.

URL of deployed dev instance (used for testing):

  • https://___.webknossos.xyz

Steps to test:

  • I patched the code locally so that undefined is passed to the number slider and it worked as expected. One can also test it by using the front-end api to set an undefined opacity, but I think it's not crucial to reproduce this. A review should be sufficient.

Issues:


Copy link
Contributor

@MichaelBuessemeyer MichaelBuessemeyer left a comment

Choose a reason for hiding this comment

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

I just tried to find the bug myself and found out that the NumberInput isn`t validated good enough.

Just typing an empty string into the numberfield and then defocusing the browsers window crashes wk.

A possible solution is adding _.isNumber(_value) to the if check in line 27. This should make sure, that no none number value is saved as a number setting.

That way your workaround might not be necessary.

What do you think about this solution @philippotto?

@philippotto
Copy link
Member Author

philippotto commented Aug 10, 2020

I also added a validation check to the onblur code. Thanks for finding this bug, @MichaelBuessemeyer 🙏

Copy link
Contributor

@MichaelBuessemeyer MichaelBuessemeyer left a comment

Choose a reason for hiding this comment

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

LGTM 🎉

@bulldozer-boy bulldozer-boy bot merged commit b8fc1dd into master Aug 10, 2020
@bulldozer-boy bulldozer-boy bot deleted the fix-broken-slider branch August 10, 2020 11:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants