Issue479 - Fix Vertical Slider regression #481

Marc-Andre-Rivet · 2019-03-07T20:34:16Z

Fixes #479

The test fails without the fix: https://circleci.com/gh/plotly/dash-core-components/3577

Exception: ('browser error logged during test', {'level': 'SEVERE', 'message': "http://localhost:8050/_dash-component-suites/dash_renderer/react-dom@15.4.2.min.js?v=0.20.0&m=1551989220 13:5540 Uncaught TypeError: Cannot read property 'toFixed' of undefined", 'source': 'javascript', 'timestamp': 1551989561259})

Marc-Andre-Rivet · 2019-03-07T20:35:42Z

src/components/Slider.react.js

                data-dash-is-loading={
                    (loading_state && loading_state.is_loading) || undefined
                }
+                style={{display: 'initial'}}


overrides browser default styling display: block for default div behavior -- this allows the child to have the correct size

🤔 IE11? https://developer.mozilla.org/en-US/docs/Web/CSS/initial#Browser_compatibility

A little fiddling indicates that for display, initial means inline? Can we use that?

Oh.. did not realize IE11 did not support initial. Thanks for checking it out.

Shouldn't the display be inline-block anyway, like here?

dash-core-components/src/components/DatePickerRange.react.js

Line 130 in 8ccbb40

display: 'inline-block',

So.. realizing that maybe just doing style={vertical ? { height: '100%' } : {}} on the wrapper div might be just as effective. This mimics the nested slider's styling.

Marc-Andre-Rivet · 2019-03-07T20:36:32Z

test/IntegrationTests.py

+        # store the very last timestamp
+        self.last_timestamp = last_timestamp
+
+        return filtered


To be called once per test (at least once in the teardown phase) to get the test-related browser errors

I really like this solution - much cleaner than the window.tests hack, and (cc @valentijnnieman) it will catch errors during page load as well, presumably.

I'm curious though, why a separate class, rather than just a method of IntegrationTests?

No specific reason. Found it while looking for the capabilities finer points in Python and liked it. Did not stop to consider if it made more or less sense to have it standalone. I can imagine wanting to make this richer and keeping it in a class sort of marginally makes sense in that context.

Looks good :-) I think it would make more sense to me to have it as a method if IntegrationTests instead of a separate class - just to keep everything in one place.

Sure. I can move this into IntegrationTests

Marc-Andre-Rivet · 2019-03-07T20:36:43Z

test/IntegrationTests.py

            options.binary_location = os.environ['DASH_TEST_CHROMEPATH']

-        cls.driver = webdriver.Chrome(chrome_options=options)
+        cls.driver = webdriver.Chrome(options=options, desired_capabilities=capabilities)


Settings to get browser errors

Marc-Andre-Rivet · 2019-03-07T20:37:41Z

test/test_integration.py

+        v_slider.click()
+
+        for entry in self.log_facade.get_log():
+            raise Exception('browser error logged during test', entry)


If there's a message here, something went wrong. Not doing this for all tests as lots of tests have issues at the moment..

https://github.com/plotly/dash-core/issues/51

I'm wondering if this test shouldn't be in the test_gallery test, since that's where most of the snapshots of components are taken. Although, I would rather see the test_gallery test eventually be broken up into specific tests per component, so I guess this is a good start! :-)

I think the gallery snapshot approach is a bad idea in general. When I see a diff I want that diff to be isolated and linked to as little dependencies as possible. If the vertical slider breaks, it's obvious. And no wondering if there's multiple issues in the single diff.

Agreed - so many times I've had to look at a diff where something was added in the middle of a long page... and percy has no idea that it's all the same content just shifted down, so I have to try and do that diffing in my head. Much better to break these up into single-purpose images, and not worry about making a huge total number of images.

alexcjohnson · 2019-03-07T23:27:16Z

test/IntegrationTests.py


        options = Options()
+        capabilities = DesiredCapabilities.CHROME
+        capabilities['loggingPrefs'] = {'browser': 'SEVERE'}


I tried in vain to find docs for this, did you? If this config does what's needed here that's fine for this PR, I'm just curious if we might like other settings in other cases, to get messages that don't quite rise to the level of SEVERE

https://github.com/SeleniumHQ/selenium/wiki/DesiredCapabilities#loggingpreferences-json-object

Available values for most loggers are "OFF", "SEVERE", "WARNING", "INFO", "CONFIG", "FINE", "FINER", "FINEST", "ALL".

mmm yeah, I saw that one, but it also says

See the documentation of each driver for what browser specific logging components are available.

Which I was hoping existed somewhere and would tell us something about what kind of messages are included in each level... oh well, trial and error I guess.

valentijnnieman

Haven't run the new test on my machine, just looked at the code and commented here and there. If other reviews are 👍 then I'm fine with it too!

valentijnnieman · 2019-03-08T16:27:03Z

src/components/Slider.react.js

                data-dash-is-loading={
                    (loading_state && loading_state.is_loading) || undefined
                }
+                style={{display: 'initial'}}


Shouldn't the display be inline-block anyway, like here?

dash-core-components/src/components/DatePickerRange.react.js

Line 130 in 8ccbb40

display: 'inline-block',

valentijnnieman · 2019-03-08T16:28:34Z

test/IntegrationTests.py

+        # store the very last timestamp
+        self.last_timestamp = last_timestamp
+
+        return filtered


Looks good :-) I think it would make more sense to me to have it as a method if IntegrationTests instead of a separate class - just to keep everything in one place.

valentijnnieman · 2019-03-08T16:33:02Z

test/test_integration.py

+        v_slider.click()
+
+        for entry in self.log_facade.get_log():
+            raise Exception('browser error logged during test', entry)


I'm wondering if this test shouldn't be in the test_gallery test, since that's where most of the snapshots of components are taken. Although, I would rather see the test_gallery test eventually be broken up into specific tests per component, so I guess this is a good start! :-)

…ly/dash-core-components into issue479-vertical-slider-regression

alexcjohnson

💃

Marc-André Rivet added 8 commits March 7, 2019 11:32

add vertical slider test without fix

6e3ba9a

fix flake

c8065a9

more precise slider selector

f261f57

update tests w/ console error watcher

1c9dc1b

fix lint

1faedfc

fix vertical slider

29abf7b

undo useless changes

a71633b

prettier

fcc23ca

Marc-Andre-Rivet commented Mar 7, 2019

View reviewed changes

Marc-Andre-Rivet requested review from alexcjohnson and valentijnnieman March 7, 2019 20:44

Marc-Andre-Rivet marked this pull request as ready for review March 7, 2019 20:44

alexcjohnson reviewed Mar 7, 2019

View reviewed changes

change initial for inline

ae058b8

valentijnnieman reviewed Mar 8, 2019

View reviewed changes

Marc-Andre-Rivet and others added 5 commits March 8, 2019 11:42

Update CHANGELOG.md

51ac3e6

style vertical case with height

b79935b

Merge branch 'issue479-vertical-slider-regression' of github.com:plot…

bf63acd

…ly/dash-core-components into issue479-vertical-slider-regression

lint

ae84539

refactor logs

8341d22

alexcjohnson approved these changes Mar 8, 2019

View reviewed changes

Marc-Andre-Rivet merged commit 77d327f into master Mar 8, 2019

Marc-Andre-Rivet deleted the issue479-vertical-slider-regression branch March 8, 2019 19:21

Marc-Andre-Rivet mentioned this pull request Mar 11, 2019

Issue 485 - Slider className / external styling regression #486

Merged

Uh oh!

Issue479 - Fix Vertical Slider regression #481

Issue479 - Fix Vertical Slider regression #481

Uh oh!

Conversation

Marc-Andre-Rivet commented Mar 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Marc-Andre-Rivet Mar 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Marc-Andre-Rivet Mar 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

valentijnnieman left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexcjohnson left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Marc-Andre-Rivet commented Mar 7, 2019 •

edited

Loading

Marc-Andre-Rivet Mar 8, 2019 •

edited

Loading

Marc-Andre-Rivet Mar 8, 2019 •

edited

Loading