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

Input component updates #356

Merged
merged 13 commits into from Nov 5, 2018

Conversation

Projects
None yet
3 participants
@valentijnnieman
Copy link
Contributor

commented Oct 31, 2018

edit:
This PR is becoming much bigger than expected, apologies in advance if the commit history or comments are not super clear.

This updates the Input component with a new debounce prop that determines when setProps is called, should fix #169. It also adds a bunch of unit tests, and fixes an issue where props coming from dash-renderer would overwrite the current state of the input. That last bug is more common than we think - components that use state as well as setProps get out of sync often.

Here's an example app with the new debounce prop:

import dash
from dash.dependencies import Input, Output
import dash_core_components as dcc
import dash_html_components as html

app = dash.Dash()
app.scripts.config.serve_locally = True

app.layout = html.Div([
    dcc.Input(placeholder='Enter a value...',
        type='number',
        value=0,
        step=0.01,
        debounce=True,
        id='input_id',
        style={'float': 'left'}),
    html.Div(id='my-div')
])

@app.callback(
    Output(component_id='my-div', component_property='children'),
    [Input(component_id='input_id', component_property='value')]
)
def update_output_div(input_value):
    return 'You\'ve entered (+1) {}'.format(input_value+1)

if __name__ == '__main__':
    app.run_server()

It also fixes #292 by fixing the step prop and #173 by not updating value if < min or > max.

Also renamed a couple of props that weren't being picked up by React, similarly to #348.

@@ -32,7 +32,7 @@ def __init__(self, id=Component.REQUIRED, pathname=Component.UNDEFINED, search=C
_locals.update(kwargs) # For wildcard attrs
args = {k: _locals[k] for k in _explicit_args if k != 'children'}

for k in [u'id']:
for k in ['id']:

This comment has been minimized.

Copy link
@valentijnnieman

valentijnnieman Oct 31, 2018

Author Contributor

Not sure why this is happening to be honest.

This comment has been minimized.

Copy link
@T4rk1n

T4rk1n Oct 31, 2018

Contributor

That the dash generate component, I think I generated from py3 and it added that unicode, I regened from py2 and they were gone.

This comment has been minimized.

Copy link
@valentijnnieman

valentijnnieman Oct 31, 2018

Author Contributor

Ah ok! That's probably fine then!

expect(input.props().value).toBeDefined();
expect(input.props().value).toEqual(defaultProps.value);

// test if id is in the actual HTML string

This comment has been minimized.

Copy link
@Marc-Andre-Rivet

Marc-Andre-Rivet Oct 31, 2018

Contributor

value, not id

// props, if dash-renderer is not informed of prop updates
input.setProps({value: 'initial value'});

expect(input.find('input').getNode().value).toEqual('new value');

This comment has been minimized.

Copy link
@Marc-Andre-Rivet

Marc-Andre-Rivet Oct 31, 2018

Contributor

Could you explain what is being tested here?

This comment has been minimized.

Copy link
@valentijnnieman

valentijnnieman Oct 31, 2018

Author Contributor

Yes! In some cases, dash-renderer will need to re-render the entire layout. It will, at that point, push the props in the components it wants to render. If, Dash isn't informed of any prop updates (in the case were setProps() is not being used, but the component's internal state is) then it will push in the old prop, or the initial prop.

@Marc-Andre-Rivet
Copy link
Contributor

left a comment

Lgtm. There's one outstanding comment left where it's written 'id' instead of 'value'. Otherwise looks fine.

@valentijnnieman

This comment has been minimized.

Copy link
Contributor Author

commented Nov 1, 2018

There are still some open issues on the Input component, figured I'd get some fixes for those in. This should close #173 now (when merge conflicts are resolved). I intend to look into #169 as well, while I'm at it. I'll change the status of this to WIP!

@valentijnnieman valentijnnieman changed the title Unit test for Input component and some prop renames [WIP] Input component updates Nov 1, 2018

@T4rk1n

This comment has been minimized.

Copy link
Contributor

commented Nov 2, 2018

@valentijnnieman this fix #362, can you add this test to test_integration:

    def test_input_lose_focus(self):
        app = dash.Dash(__name__)
        app.layout = html.Div([
            dcc.Input(
                id='input',
                value='initial value'
            ),
            dcc.Input(id='input-2'),
            html.Div(
                html.Div([
                    1.5,
                    None,
                    'string',
                    html.Div(id='output-1')
                ])
            )
        ])

        @app.callback(Output('output-1', 'children'), [Input('input', 'value')])
        def update_output(value):
            return value

        self.startServer(app)

        self.wait_for_text_to_equal('#output-1', 'initial value')

        input1 = self.wait_for_element_by_css_selector('#input')
        input1.clear()
        clicker = self.wait_for_element_by_css_selector('#input-2')
        clicker.send_keys('bad')

        input1.send_keys('hello world')
        time.sleep(1)

        self.assertEqual('hello world', input1.get_attribute('value'))

💃

@valentijnnieman

This comment has been minimized.

Copy link
Contributor Author

commented Nov 2, 2018

@T4rk1n Sorry, I'm not sure what this test is testing, could you explain? I ask because I want to try to split up tests into (1) Unit tests that go into test/unit/{componentName}.test.js and (2) Integration tests that use an actual Dash app that go into test/test_integration.py. I'm not sure if your test should be a unit test or an integration test.

@T4rk1n

This comment has been minimized.

Copy link
Contributor

commented Nov 3, 2018

It's an integration test for a bug introduced by n_blur and setState in componentWillReceiveProps, when the component lost focus it would prepend the value with the initial value.

So I wrote this test that change the focus to another input and assert the first input is still the same value, it failed with the dcc version on master. wait_for_text_to_equal is actually wait_for_text_to_be_present, so the previous tests in dash would pass with those percy diffs. With your changes to props handling this test passed.

@valentijnnieman

This comment has been minimized.

Copy link
Contributor Author

commented Nov 5, 2018

@T4rk1n In that case, the behaviour of new props and setState in componentWillReceiveProps is already being tested in the unit test, I feel. I'm kind of wary of adding (slow) integration tests that test behaviour that is (or should be) already tested in that component's unit tests. If you want you can make a PR later that adds this test, or better yet, update the new unit tests!

valentijnnieman added some commits Nov 5, 2018

@T4rk1n

This comment has been minimized.

Copy link
Contributor

commented Nov 5, 2018

Testing time used to be not so slow, I investigated and found a few issues. Dropped the test time from ~8:30 avg to ~4:30..

The test is already written and proven to fail, it only adds at most a couple seconds. I've been hunting this bug for two weeks, finally found the culprit and wrote the test with failing behavior with the intention to fix it, was out of idea and noticed you changed things here so I tried and it passed. I'd prefer not having to rewrite it, so I'll merge master after this PR on the failing branch and add it in another PR.

Have yet to take a good look at the new unit tests, I don't know much about jest/enzyme. They look good at first glance with the mocked setProps.

@valentijnnieman valentijnnieman changed the title [WIP] Input component updates Input component updates Nov 5, 2018

@valentijnnieman

This comment has been minimized.

Copy link
Contributor Author

commented Nov 5, 2018

@T4rk1n Yeah the time isn't really that much of an issue for me, it's more that I think that what you're testing in your test is already covered in the unit tests now. I want to really start dividing tests between unit and integration, testing the behaviour of the component in the unit tests and the integration with Dash in the integration tests. That way, we can more easily do test driven development, writing unit tests that fail and make them pass, then assert that the behaviour in Dash stays the same later in the integration tests. It's just a lot easier to do that as opposed to make a change to the code, do a build:js and build:py, and then fire up the Selenium tests each time to make sure it works. With Jest you're testing the src code, not the build. Having said all that, I don't think there are a lot of Input integration tests, so that's something that we should change.

## [0.36.0] - 2018-11-01
### Fixed
### Fixe

This comment has been minimized.

Copy link
@Marc-Andre-Rivet

Marc-Andre-Rivet Nov 5, 2018

Contributor

The d is gone!

This comment has been minimized.

Copy link
@valentijnnieman

valentijnnieman Nov 5, 2018

Author Contributor

Why is the d gone?

This comment has been minimized.

Copy link
@Marc-Andre-Rivet

Marc-Andre-Rivet Nov 5, 2018

Contributor

I unno!

setProps({value: e.target.value});
}
const newValue = e.target.value;
if (((min || max) && newValue < min) || newValue > max) {

This comment has been minimized.

Copy link
@Marc-Andre-Rivet

Marc-Andre-Rivet Nov 5, 2018

Contributor

I think this check is wrong. 0 is falsy and max is compared always

test('Input can not be updated lower than props.min', () => {
input
.find('input')
.simulate('change', {target: {value: props.min - 1}});

This comment has been minimized.

Copy link
@Marc-Andre-Rivet

Marc-Andre-Rivet Nov 5, 2018

Contributor

As discussed, these tests are incorrect in so far as they are sending a number to the component when an actual onchange event would send a string

input = mount(<Input type="number" value={0} />);
});
test('Input can be updated', () => {
input.find('input').simulate('change', {target: {value: -1}});

This comment has been minimized.

Copy link
@Marc-Andre-Rivet

Marc-Andre-Rivet Nov 5, 2018

Contributor

make it a string

test('Input can be updated', () => {
input.find('input').simulate('change', {target: {value: -1}});
expect(Number(input.find('input').getNode().value)).toEqual(-1);
input.find('input').simulate('change', {target: {value: 100}});

This comment has been minimized.

Copy link
@Marc-Andre-Rivet

Marc-Andre-Rivet Nov 5, 2018

Contributor

make it a string

@valentijnnieman valentijnnieman merged commit c05b927 into master Nov 5, 2018

4 checks passed

ci/circleci: python-2.7 Your tests passed on CircleCI!
Details
ci/circleci: python-3.6 Your tests passed on CircleCI!
Details
ci/circleci: python-3.7 Your tests passed on CircleCI!
Details
percy/dash-core-components Visual review automatically approved, no visual changes found.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.