Number editor return number. #541
Conversation
|
Thanks for this! Never noticed the Number control directly copied the String control, weird. I am a bit concerned about breaking current implementations which may be depending on this field producing a string. |
|
That's fair. And in fact, I just realised I had my blinders on when I created this.... Rather than modifying the NumberControl, maybe adding an IntegerControl (parseInt) and a FloatControl (parseFloat); and then a documentation note saying the "NumberControl mirrors the HTML input number behaviour of returning a string" is a better option? |
|
What do you think about just making the return value type configurable for the Number Control? Also, if we start explicitly handling floats, we should use the I'm thinking the following config additions for the Number Control:
|
|
Sounds like a good plan to me :-) I'm about to be sans computer for a couple of weeks, so this might be on hold for a few weeks from my front, but I'm happy to continue with it after (or hopefully can get it done before then) - just thought I'd let you know about the delay in advance. |
|
Thanks for the heads up! Sounds good. |
Adding support for `min`, `max`, `step` on the input element and adding `valueType` for specifying the return type.
There was a problem hiding this comment.
Thanks for getting this in @bruce-one! Just a few code style changes (with my apologies that we don't have a code style guide yet) and this should be good to merge.
| handleChange = (e) => { | ||
| this.props.onChange(e.target.value); | ||
| const valueType = this.props.field.get('valueType') | ||
| let value |
There was a problem hiding this comment.
We generally want to avoid mutating variables - you could instead set onChange as a new const and call it in each condition, without storing the value.
| const min = field.get('min') | ||
| const max = field.get('max') | ||
| const step = field.get('step') | ||
| return <input type="number" id={forID} value={value || ''} step={step == null ? (field.get('valueType') === 'int' ? 1 : '') : step} min={min == null ? '' : min} max={max == null ? '' : max} onChange={this.handleChange} />; |
There was a problem hiding this comment.
We should break this down to one attribute per line.
There was a problem hiding this comment.
Also, you can simply pass a second argument to .get in order to set a default value if the value you're looking for isn't present, which should allow you to remove most of the || and ternaries which are currently checking for defaults.
| handleChange = (e) => { | ||
| this.props.onChange(e.target.value); | ||
| const valueType = this.props.field.get('valueType') |
There was a problem hiding this comment.
We still use semicolons everywhere, so we'll need them here, too. We'll probably consider dropping them at some point, I've certainly considered it.
|
Thanks for the feedback :-) Everything's done, I think? |
Adding support for `min`, `max`, `step` on the input element and adding `valueType` for specifying the return type, so the `NumberControl` can return actual numbers.
- Summary
Number editor creates strings not numbers. Use parseInt to ensure a number is created.
- Test plan
Tested manually, issue noticed because Jekyll's sort complained. (Couldn't find the widget tests? But didn't look too hard :-s It's a pretty trivial change...)
- Description for the changelog
Ensure Number widget saves values as numbers not strings.
- A picture of a cute animal (not mandatory but encouraged)