-
-
Notifications
You must be signed in to change notification settings - Fork 145
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice 💃 have just one suggestion, up to you if you want to include that in this PR.
@@ -41,6 +41,20 @@ export default class Input extends Component { | |||
if (fireEvent) { | |||
fireEvent({event: 'blur'}); | |||
} | |||
if (setProps) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A small improvement here could be that if setProps
is set, the component doesn't need to keep state itself, i.e. if setProps
is set you don't need to fire setState
. If you were to implement that, you'd also need to add that logic to componentWillReceiveProps
. We had a small discussion about that in #331 - it won't cause any bugs like that here but it's still a small gain to not keep unnecessary state in this component. Up to you if you want to add that now, we could always add it later if you want to just merge this first.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But it's not using the state.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah I was looking at line 29. That's something for another time then, feel free to merge!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a note about that in our react dev guide ? In the SuggestionsInput, I was using componentWillReceiveProps
to setState on the value, but also setState before calling setProps on the value and the value in the props and state was always different so it lead to a bug where no suggestions beyond the first was registered so it a very valid concern to think about when developing dash component.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exactly, like I mentioned above I found a bug with that recently too in #331. In the Input component I don't think it'll lead to any bugs though. But it's not in the React for Python devs guide, no. Would be good to add that!
9f023b9
to
b6dba86
Compare
@@ -37,7 +37,7 @@ def __init__(self, id=Component.REQUIRED, storage_type=Component.UNDEFINED, data | |||
_locals.update(kwargs) # For wildcard attrs | |||
args = {k: _locals[k] for k in _explicit_args if k != 'children'} | |||
|
|||
for k in ['id']: | |||
for k in [u'id']: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why did you do this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You commenting on auto generated code, I didn't do that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah ok. Sorry if it was rude.
Add four props to dcc.Input:
n_submit
, The number of timesenter
was pressed when the component had focus.n_submit_timestamp
The last timestamp enter was pressed.n_blur
, The number of times the component lost focus.n_blur_timestamp
, The last time the component lost focus.resolves #230