Skip to content
This repository has been archived by the owner on Jun 4, 2024. It is now read-only.

Issue148 - 0 children regression in dash==0.40.0 #150

Merged
merged 6 commits into from
Apr 11, 2019

Conversation

Marc-Andre-Rivet
Copy link
Contributor

@Marc-Andre-Rivet Marc-Andre-Rivet commented Apr 11, 2019

FIxes #148.

Found the issue in dash-docs percy deltas in https://percy.io/plotly/dash-docs/builds/1719186
image

Add test for array case children=[0], this passes with the existing code: 0602a15

Add test for non-array case children=0, this fails with the existing code: d48e81d

Fix: b600b8c

@@ -33,7 +33,7 @@ const createContainer = component => isSimpleComponent(component) ?

class TreeContainer extends Component {
getChildren(components) {
if (!components) {
if (isNil(components)) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

0 is falsy..

@@ -475,6 +475,32 @@ def test_initial_state(self):

assert_clean_console(self)

def test_array_of_nully_child(self):
app = Dash(__name__)
app.layout = html.Div(id='nully-wrapper', children=[0])
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This worked and needs to keep working

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice. I'd call this falsy though, not nully.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good point


def test_of_nully_child(self):
app = Dash(__name__)
app.layout = html.Div(id='nully-wrapper', children=0)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the regression case

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

too late for the comment, but this should be merged with the test_initial_state.

@Marc-Andre-Rivet Marc-Andre-Rivet changed the title Issue148 - 0 children regression Issue148 - 0 children regression in dash==0.40.0 Apr 11, 2019
@@ -104,7 +104,7 @@ def request_queue_assertions(

if expected_length is not None:
self.assertEqual(len(request_queue), expected_length)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh noes, trailing whitespace courtesy of @chriddyp ??? 😅

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no way!! 🙊 🙊 🙊

self.wait_for_element_by_css_selector('#nully-wrapper')
wrapper = self.driver.find_element_by_id('nully-wrapper')

self.assertEqual(wrapper.get_attribute('innerHTML'), '0')
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't this whole section just be self.wait_for_text_to_equal('#nully-wrapper', '0')?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hum.. wasn't aware of that one. Looks like wait_for_text_to_equal uses value and that would mean it's only meant to be used with <input /> elements.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

value or .text:

(str(self.wait_for_element_by_css_selector(selector).text)

Copy link
Contributor Author

@Marc-Andre-Rivet Marc-Andre-Rivet Apr 11, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Embarrassing. I can't read code it seems. Will update to use suggested.

Copy link
Collaborator

@alexcjohnson alexcjohnson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for catching & fixing this! Only pedantic comments 💃

@Marc-Andre-Rivet Marc-Andre-Rivet merged commit d2d902b into master Apr 11, 2019
@Marc-Andre-Rivet Marc-Andre-Rivet deleted the issue148-nully-regression branch April 11, 2019 02:19
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants