-
-
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.
Thanks @inytar - this looks great!
Can you exclude the build artifact changes from this PR? (You're about a day early before we do this automatically #659) That would be everything except src/
and tests/
And as far as tests go, I see the empty block in unit/Dropdown.test.js
, I'm guessing that was just added when you needed to edit that file to get tests to pass, and you were planning to come back to it... but I think this would be better served by an integration test. Let's make a new folder tests/integration/dropdown
and run a test against the same app you showed in the lead comment.
@alexcjohnson thanks for the review. I'll remove the artifacts. |
A good example is wait.until(lambda: len(dash_duo.find_elements('<option selector>')) == <expected number>)
assert dash_duo.find_element('<option selector>').text == <expected option> |
) | ||
def update_output(search_value): | ||
call_count.value += 1 | ||
return 'search_value="{}"'.format(search_value) |
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 sorry, I missed this earlier - I think the test should use the same structure as the example app you gave, which is updating options
from search_value
in the same component. The reason this is important is (a) that's the primary use case for this prop, and (b) it will cause the dropdown to rerender, and we need to ensure that when it does the correct options are visible. I can certainly imagine errors in the component where on render it loses focus, or for some other reason it stops displaying the option list.
(BTW nota big deal but you can probably get rid of all the call_count
stuff, that doesn't really matter for this test, we only care about what's going on within the browser)
@alexcjohnson I miss read your first comment and thought you just wanted a basic integration test. I've kept this test because I think it's good to keep a basic test that is easier to debug/understand if something fails. I've also added a test that updates the options dynamically from the server, I hope this is what you meant. |
import dash_core_components as dcc | ||
|
||
|
||
def test_ddsv001_dynamic_options(dash_duo): |
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.
sorry I was not clear about the suggestion. the TCID added in the previous test should be unique. more details refer to Notes #2 in example. https://dash.plot.ly/testing
this one should be sth like dddo001
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.
Thanks for bearing with our suggestions @inytar - very nicely done. Just the suggestion from @byronz about TCID uniqueness https://github.com/plotly/dash-core-components/pull/660/files#r328776554 and this will be ready to go! 💃
Oh, the linter had some complaints as well:
It looks like js linting also complained but that somehow doesn't cause the test run to fail... so don't worry about that, we'll get that separately. |
Thanks for looking at this and the fast reviews! I enjoyed the process and hope I get to work with plotly dash more in the future. |
Btw, this is the related example for the documentation. I'd be very happy if this can also be added. |
oops I forgot to ask for a changelog entry - added here: d750da6 |
This PR adds the
search_value
property to theDropdown
component. Using thesearch_value
you can do server side filtering of theoptions
as shown in the following example:This example will also be added to dash-cocs.
An example of a use-case of this feature is described in this forum topic.