fixes plotly/dash-core_components#169 #277

Bachibouzouk · 2018-08-24T18:17:23Z

fixes plotly/dash-core_components#169

Fix the decimal problem by using onBlur instead of onChange. However this solution works only if the line value={value} in the render function is commented.

…ver this solution works only if the line `value={value}` in the `render` function is commented.

valentijnnieman · 2018-08-24T20:35:45Z

src/components/Input.react.js

@@ -25,25 +25,29 @@ export default class Input extends Component {
            setProps,
            type
        } = this.props;
-        const {value} = this.state;
+        //const {value} = this.state;


If you comment this out, it will mean that a Dash user cannot provide the value prop to the dcc.Input component (or rather, it will not do anything). You can see in the constructor of the class that this.state.value is set to props.value, and below in <input />, it gets set to the value attribute: <input value={value} />.

valentijnnieman · 2018-08-24T20:44:22Z

src/components/Input.react.js

-                }}
-                value={value}
+                }
+                //value={value}


The reason that this doesn't work without removing the value={value}, is probably because onBlur only fires the update, well, on blur, as opposed to onChange which fires every change. Setting value here will overwrite anything you put in the input (try having just <input value={value} /> in there, and you'll see what I mean - it is as if it's 'locked' to whatever is put in as the value prop) so onBlurwill fire it's update on blur, but because the input is 'locked', it fires the update with the same value. Does that make sense?

Yes it make sense, maybe it could work with another variable, i.e. typed for example. I will try

I Added a prop typed to the state (Is it the proper way to say this?), this gets updated by onChange , the valueprop takes the value of thetypeprop whenonBlur``gets fired, so now it works (at least on windows 10)

…s the prop `value` by `typed`, that way we don't need to comment out the line `value={value}`

Bachibouzouk · 2018-08-29T08:30:30Z

When I try to get the details of the Percy tests I get the following error :

Error: Ember Data Request GET /api/v1/organizations/plotly returned a 403
Payload (Empty Content-Type)
[object Object]

I guess I should update the changelog by pulling the latest version of master before merging?

chriddyp · 2018-08-29T12:41:04Z

Error: Ember Data Request GET /api/v1/organizations/plotly returned a 403

I'll need to add you to percy first

I guess I should update the changelog by pulling the latest version of master before merging?

Yeah, or rebase on master and then update the changelog

Bachibouzouk · 2018-08-29T15:34:01Z

Couldn't we use <NumericInput/> instead of <input type='number'/> ? The first one provides the little stepper on the right of the input box for edge, whereas <input type='number'/> doesn't

https://www.npmjs.com/package/react-numeric-input

…epper which gets rendered in firefox for an `<input type=number>`

chriddyp · 2018-08-30T17:42:32Z

Couldn't we use instead of ?

For this bug, I'd rather write the logic ourselves rather than introduce more dependencies

Bachibouzouk · 2018-08-30T21:24:45Z

The fix I found works except that it requires that the dcc.Input has a value prop defined in dash. I think this comes from the

const {value} = this.state.value;

return (
            <input
                 // some code
                 value={value}

              />
);

instead of

const {value} = this.state;

return (
            <input
                 // some code
                 value={value}

              />
);
...

I did that because I introduced another prop in this.state in the constructor of the component to be able to not rerender value with onChange. If I set const value = this.state; in my modified code, then it only displays the last key strokes Inside the input layout.

Bachibouzouk · 2018-08-30T23:04:49Z

Assigning value={value} generates a warning : A component is changing an uncontrolled input of type text to be controlled. Input elements should not switch from uncontrolled to controlled (or vice versa). Decide between using a controlled or uncontrolled input element for the lifetime of the component. More info: https://fb.me/react-controlled-components

For the logic of this component do we want that the value gets changed at every keystroke? or only when the user is done writing?

chriddyp · 2018-08-30T23:31:57Z

Considering the case for decimal numbers, trying to type out 0.1005 (and thereby needing to type 0.10, a representation that doesn't exist in Python or Javascript), we have two options:

Fire the callback at 0, 0.1, 0.1005. Keeping an internal representation of the value as a string (so our state is 0, 0., 0.1, 0.10, 0.100, 0.1005) and setting that value as the <input value={this.state.value}/> and only calling setProps when there aren't any trailing zeros after the decimal place. This would fix the bug and introduce no new properties
Alternatively, we can introduce a new property similar to the Slider's updatemode: PropTypes.oneOfType(PropTypes.oneOf(['keypress', 'blur', 'enter']), PropTypes.arrayOf(PropTypes.oneOf['keypress', 'blur', 'enter'])). That is, the user could specify updatemode: 'keypress' or updatemode: 'blur' or combinations, e.g. updatemode: ['blur', 'enter']. Then, the numerical input developers could set updatemode: ['blur', 'enter'] and not worry about the keypress case. For backwards compatability, updatemode: 'keypress' would have to remain the default.

Both solutions would be valid. (1) is strictly correct but (2) seems like it would satisfy the users who reported the bug in #169 and provide a new feature in general.

…r dash callback on 'value' changes at at every keystroke, still some bug when the `type` prop is set to `number`) or blur (trigger dash callback on 'value' changes only when the user click somewhere else or press tab). When the `type` prop is set to `number`, then pressing on the lateral steppers button will immediately trigger a callback on 'value' change.

# Conflicts: # src/components/Input.react.js

Bachibouzouk · 2018-09-09T00:54:33Z

@valentijnnieman I am not sure how I should make keypress the default value of the updatemode prop?

--edit : nevermind I found that by looking at the slider code :)

… the user cannot type anything else than [0-9], the comma, the dot and the minus sign. The minus sign must be in first place and the comma and the dot are limited to one instance only and are mutually exclusive (if there is a dot, one cannot type a comma)

…nput/>`

Bachibouzouk · 2018-09-12T10:57:09Z

One thing that I am unable to control now is that the used is allowed to feed a string with content not matching a number format, i.e. '12.2,12'

dcc.Input(
        id='input3',
        type='number',
        value='12.2,12', \\this value will be assigned but not displayed Inside the component
        updatemode='blur',
)

Another thing (maybe linked) is that the user is allowed to type in a wrong number format (i.e. it displays it although the value of the value prop is set to ''. This was not the case when I tested the code in a dummy example on codepen.

valentijnnieman

I've left you a couple of comments! Looks like it's getting there though! :)

valentijnnieman · 2018-09-12T15:44:53Z

src/components/Input.react.js

+            <input
+                onBlur={() => {
+                    const newValue = formatValue(this.state.value, type);
+                    if (updatemode === 'blur') {


Here, if updatemode is an array (i.e. ['blur', 'enter']) this will not trigger. The best way to do this would probably be to first see if updatemode is an array (you could use Ramda's is function for that, something like R.is(Array, updatemode)) and then check if that array contains blur (Ramda has a contains function that can do that easily)

Or maybe even if (updatemode === 'blur' || R.contains('blur', updatemode)) {} will work!

actually I implemented with updatemode being blur (update when pressing tab or clicking away) or keypress (update at everykeystrokes), I wasn't sure the if we really need to have ['blur', 'enter'] but I kept it in the Input.Props

valentijnnieman · 2018-09-12T15:45:54Z

src/components/Input.react.js

+                onChange={e => {
+                    const newValue = formatValue(e.target.value, type);
+                    this.setState({value: newValue});
+                    if (updatemode === 'keypress') {


Similarly here - this if only works if updatemode is a string, not when it's an array.

valentijnnieman · 2018-09-12T15:50:10Z

src/components/Input.react.js

+                        ) {
+                            e.preventDefault();
+                        }
+                    }


Could you explain a bit about what this code is supposed to do? Is it really necessary?

This rejects the onKeyPress event and the onChange event is therefore not triggered. This is used to make sure the user can only press on 0-9, dot and comma and that the user cannot input Something like 0.122.,233,44.4, it prevents the use of another dot or comma when either one of them is already present

valentijnnieman · 2018-09-18T15:46:49Z

Nice! Couple of tiny things though.

You can enter multiple .'s at the start - also characters like -

Here if you enter a number and then a ., it clears the input upon entering another .

I'm not sure if the way to go here is to prevent all these types of inputs. I think having the normal behaviour of an <input /> tag is totally fine, we just have to be careful of when to send back the value to Dash. The new update methods are already a good work-around for those having trouble with inputs and floating point numbers, so maybe that's enough.

@chriddyp Could you weigh in on this?

Bachibouzouk · 2018-09-18T21:58:30Z

Here if you enter a number and then a ., it clears the input upon entering another .
It doesn't do that for me

However I also have the problem of being able to input many dots. I can fix it, but it could also be decided that it is better to leave the user able to input more freely as you suggested :)

valentijnnieman · 2018-11-07T20:28:21Z

Going to close this because the issues have been fixed in #356

Fix the decimal problem by using onBlur instead of onChange. Howe…

2c59df5

…ver this solution works only if the line `value={value}` in the `render` function is commented.

valentijnnieman reviewed Aug 24, 2018

View reviewed changes

introducted typed which gets updated by onChange and onBlur replace…

1db86aa

…s the prop `value` by `typed`, that way we don't need to comment out the line `value={value}`

Bachibouzouk added 2 commits August 30, 2018 19:26

the addition of onClick event handler solves the issue of the side st…

44e4eea

…epper which gets rendered in firefox for an `<input type=number>`

fixing the test crasher

71dc849

Bachibouzouk added 3 commits September 8, 2018 14:27

Merge branch 'master' into fix_decimal_input

c3b3f06

# Conflicts: # src/components/Input.react.js

fixed the failing eslint tests

d552406

Bachibouzouk added 2 commits September 12, 2018 12:47

added default values for the updatemode and type props of the `<i…

099dac4

…nput/>`

ran Prettier linter on the code

18b2846

valentijnnieman reviewed Sep 12, 2018

View reviewed changes

valentijnnieman mentioned this pull request Sep 12, 2018

Update npm scripts used for building, testing, etc. #299

Merged

Merge branch 'master' into fix_decimal_input

ff47e95

Bachibouzouk self-assigned this Sep 17, 2018

Bachibouzouk added 2 commits September 17, 2018 18:45

implemented change suggested by @valentijnnieman

110de52

Merge branch 'master' into fix_decimal_input

0d1274b

valentijnnieman closed this Nov 7, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fixes plotly/dash-core_components#169 #277

fixes plotly/dash-core_components#169 #277

Bachibouzouk commented Aug 24, 2018

valentijnnieman Aug 24, 2018

valentijnnieman Aug 24, 2018

Bachibouzouk Aug 27, 2018

Bachibouzouk Aug 29, 2018

Bachibouzouk commented Aug 29, 2018

chriddyp commented Aug 29, 2018

Bachibouzouk commented Aug 29, 2018

chriddyp commented Aug 30, 2018

Bachibouzouk commented Aug 30, 2018 •

edited

Bachibouzouk commented Aug 30, 2018

chriddyp commented Aug 30, 2018 •

edited

Bachibouzouk commented Sep 9, 2018 •

edited

Bachibouzouk commented Sep 12, 2018

valentijnnieman left a comment

valentijnnieman Sep 12, 2018

valentijnnieman Sep 12, 2018

Bachibouzouk Sep 12, 2018

valentijnnieman Sep 12, 2018

valentijnnieman Sep 12, 2018

Bachibouzouk Sep 12, 2018

valentijnnieman commented Sep 18, 2018

Bachibouzouk commented Sep 18, 2018

valentijnnieman commented Nov 7, 2018

fixes plotly/dash-core_components#169 #277

fixes plotly/dash-core_components#169 #277

Conversation

Bachibouzouk commented Aug 24, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bachibouzouk commented Aug 29, 2018

chriddyp commented Aug 29, 2018

Bachibouzouk commented Aug 29, 2018

chriddyp commented Aug 30, 2018

Bachibouzouk commented Aug 30, 2018 • edited

Bachibouzouk commented Aug 30, 2018

chriddyp commented Aug 30, 2018 • edited

Bachibouzouk commented Sep 9, 2018 • edited

Bachibouzouk commented Sep 12, 2018

valentijnnieman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

valentijnnieman commented Sep 18, 2018

Bachibouzouk commented Sep 18, 2018

valentijnnieman commented Nov 7, 2018

Bachibouzouk commented Aug 30, 2018 •

edited

chriddyp commented Aug 30, 2018 •

edited

Bachibouzouk commented Sep 9, 2018 •

edited