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

Handling onChange for Number Type #333

Open
minkimcello opened this issue Mar 18, 2019 · 8 comments
Open

Handling onChange for Number Type #333

minkimcello opened this issue Mar 18, 2019 · 8 comments
Labels

Comments

@minkimcello
Copy link
Contributor

I ran into an issue for number input fields with an onChange that directly invokes a set transition. This prevents the user from inputting decimals because 123. would immediately return 123 as demonstrated here:

2019-03-18 16 01 36

This isn't a problem when using an onBlur or a form, but I've been using the solution below for cases where onChange is necessary:

class NumberInput{
   get value(){
      return valueOf(this)
   }
   get state(){
      return +valueOf(this)
   }
}

Is there a more appropriate way around this?

@taras
Copy link
Member

taras commented Mar 18, 2019

@minkimcello do you have a link to the runkit example that we created?

@minkimcello
Copy link
Contributor Author

@cowboyd
Copy link
Member

cowboyd commented Mar 18, 2019

It seems like you're trying to have a natural transformation from String to Number different than the default JavaScript one: Number(string). If so, then in your inputable type you'll have to pass in that conversion as a function and use it somewhere.

@cowboyd
Copy link
Member

cowboyd commented Mar 19, 2019

To be more clear, if you want a different transformation, then you'll have use a function to do it:

For example:

class NumberInput {
  get string() {
    return valueOf(this);
  }
  get number() {
    return Number(this.string.split('.').slice(0,2).map(Number).join("."))
  }
}

let input = create(NumberInput, "5");
input.string //=> "5"
input.number //= 5
let twelveDot = input.set("12.");
twelveDot.string //=> "12."
twelveDot.number //=> 12

@taras
Copy link
Member

taras commented Mar 19, 2019

@cowboyd the issue is actually a little deeper. It shows itself in @minkimcello's example but it comes down to the fact that our current implementation of number has an initializer that can sometimes setup in state being a number and other it'll be something else.

Screen Shot 2019-03-19 at 3 35 37 PM

This makes it confusing when binding a number to an input field because it seems to work except when you start to add periods. When bound to an input, it feels broken because you can't enter a . at the end of the number.

I'm guessing that the answer here is that input fields should always be bound to String types. In addition, we might want to coerce the value into a number so a NaN would be 0. I'm not sure what the answer is here, but the current initializer logic is misleading due to allowing some inputs to be non-numbers.

@cowboyd
Copy link
Member

cowboyd commented Mar 19, 2019

I agree. I think there are two separate issues here.

  1. what to do with foo because .state should always be a number in a number. Why not use NaN for non-numbers. I'm a bit hesitant to erase NaN from the number type since it is is a part of the native JS number type. In other words, create(Number, 'foo').state //=> NaN

  2. It doesn't make sense to bind a non-string to an input.

@taras
Copy link
Member

taras commented Mar 19, 2019

I agree with both points.

  1. this is how JavaScript works
  2. this is how we should teach people

@minkimcello can you please create a PR that changes how NumberType initialization works. Change it so that create(Number, 'foo').state === NaN instead of create(Number, 'foo').state === 'foo' which we have now. It will make Number type broken for your purpose but it'll steer everyone in the right direction by causing them to use String with input fields. This would also make a good blog post for a Microstates blog that we don't have.

@cowboyd
Copy link
Member

cowboyd commented Mar 19, 2019

Slight wrinkle, you'll have to use the builtin isNaN() function since NaN !== NaN

$ node
> NaN === NaN
false
> isNaN(NaN)
true

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants