81 location url parameters #131

mjclawar · 2017-12-21T15:19:28Z

Description

This PR adds search, hash and href props to the dcc.Location component (in addition to the already-existing pathname) to better mimic the window.location API.
This addresses #81, with the caveat that dcc.Link does not have any changes to props that would allow a user to directly update search independently of hash (e.g., using dcc.Link(href="?testQuery=testValue") will wipe hash from dcc.Location. Note that this is the same behavior as window.location, so should be considered "correct", but it seemed like updating dcc.Link should be a different issue.

Implementation

The changes to dcc.Location pretty much extends the previous pathname-specific updateLocation approach. The only major difference is that if href changes in props compared to previous props and compared to window.location.href, href takes precedent over the combination of ${pathname}${searchVal}${hashVal} (see here).
gitignore was updated for IntelliJ .idea/, but can remove that if needed.
Added a package-lock.json

Tests added

This adds tests for html.A tags, dcc.Link, and html.Button clicks interacting with the updated dcc.Location component.

…arameters

chriddyp

🎉 Very nice! This looks good to me. Thank you very much for the thorough PR and the complete tests. There are just a few comments, mostly on code style conventions.

chriddyp · 2017-12-21T21:49:21Z

dash_core_components/metadata.json

@@ -1,6 +1,7 @@
 {
  "src/components/Checklist.react.js": {
    "description": "Checklist is a component that encapsulates several checkboxes.\nThe values and labels of the checklist is specified in the `options`\nproperty and the checked items are specified with the `values` property.\nEach checkbox is rendered as an input with a surrounding label.",
+    "displayName": "Checklist",


Interesting, I wonder why displayName started appearing recently. Maybe a new version of react-docgen?

chriddyp · 2017-12-21T21:51:01Z

package-lock.json

+        "async-each": "1.0.1",
+        "glob-parent": "2.0.0",
+        "inherits": "2.0.3",
+        "is-bin


👍 thanks for adding this

chriddyp · 2017-12-21T21:56:13Z

src/components/Location.react.js

@@ -1,39 +1,85 @@
 import {Component, PropTypes} from 'react';
 /* global window:true */

+/**
+ * Update and track the current window.location object through the window.history state


Maybe we can update this a little bit so that it's more readable to the Python devs who don't know much about this stuff, like:

Update and track window.location (the URL) object through the window.history state.
Use in conjunction with the dash_core_components.Link object to make apps with multiple pages.

chriddyp · 2017-12-21T21:58:19Z

src/components/Location.react.js

+         *  The name of the prop in window.location and in the component's prop
+         *
+         * @returns {boolean}
+         *  Returns true if the prop with fieldName is different and the window state needs to be updated


👍 Very nice comment, thanks for this!

chriddyp · 2017-12-21T22:02:11Z

src/components/Location.react.js

+        const checkExistsUpdateWindowLocation = (fieldName) => {
+            const propVal = props[fieldName];
+
+            if (!propVal && window.location[fieldName] && setProps) { // Prop is undefined?


we use stronger checks for undefined here like R.type(propVal) == 'Undefined' or even !R.has(props, propVal) (http://ramdajs.com/docs/#type, http://ramdajs.com/docs/#has)

It's actually a null in some cases (pathname when the page first loads, e.g.), so will update to (R.type(propVal) === 'Undefined' || propVal === null) if you prefer. I think that's why the original code had !propVal.

chriddyp · 2017-12-21T22:14:14Z

src/components/Location.react.js

+    /** href in window.location - e.g., "/my/full/pathname?myargument=1#myhash" */
+    href: PropTypes.string,
+
+    /** Refresh the page when the location is updated? */


👍 very nice, thanks for adding these

chriddyp · 2017-12-21T22:20:11Z

test/test_integration.py

+            output=Output(component_id='test-pathname', component_property='children'),
+            inputs=[Input(component_id='test-location', component_property='pathname')])
+        def update_location_on_page(pathname):
+            assert isinstance(pathname, str)


I believe that these asserts won't actually fail the test as the dash app is run in a subprocess in these tests. They'll still cause the callback to fail (which might implicitly break other assertions in the main process) but in either case we might want to put this assertion out of this context

chriddyp · 2017-12-21T22:21:50Z

test/test_integration.py

+
+        self.startServer(app=app)
+
+        def _waiter():


👍 - this is great. can we move this to test/utils.py so that we can reuse it elsewhere?

chriddyp · 2017-12-21T22:23:16Z

test/test_integration.py

+
+        # Check that pathname is updated through a Button click via props
+        self.driver.find_element_by_id('test-button').click()
+        time.sleep(1)  # Need to wait for the callback to fire TODO is there a better way to wait?


There isn't really, besides using a wait_for with whatever you are waiting for

chriddyp · 2017-12-21T22:24:06Z

test/test_integration.py

+        time.sleep(1)  # Need to wait for the callback to fire TODO is there a better way to wait?
+
+        self.snapshot('link -- /new/pathname?testQuery=testValue')
+        self.assertEqual(self.driver.find_element_by_id('test-pathname').text, '/new/pathname')


for example, you should be able to remove the time.sleep and replace it with a wait_for here and place the snapshot below the assertions

chriddyp · 2017-12-21T22:27:28Z

This addresses #81, with the caveat that dcc.Link does not have any changes to props that would allow a user to directly update search independently of hash (e.g., using dcc.Link(href="?testQuery=testValue") will wipe hash from dcc.Location. Note that this is the same behavior as window.location, so should be considered "correct", but it seemed like updating dcc.Link should be a different issue.

Could you open up a new issue about this? My guess is that we are OK here. I believe that the main use for dcc.Link will be just to set the href.

…ramda for type checking; batch updates for setProps vs each field sequentially

chriddyp · 2017-12-22T19:24:18Z

@mjclawar - Let me know when you're ready for another review!

mjclawar · 2017-12-23T12:39:16Z

All set for another review.

mjclawar · 2017-12-26T13:28:39Z

@chriddyp had to push a change to uncomment the rest of the test suite. I think all the changes you requested should be in there now!

chriddyp · 2018-01-02T21:19:33Z

Thanks! It looks like tests are failing because of some dependency mismatched. I'll make a separate PR to lock down these versions in tests

chriddyp · 2018-01-02T21:30:34Z

This looks good to me @mjclawar . Once we get the tests to pass (hopefully through #136 ), I'm happy to merge this.

mjclawar · 2018-01-02T21:37:02Z

Thanks -- wasn't sure why the tests were failing on CircleCI. Happy new year! 🎉

chriddyp · 2018-01-02T21:42:55Z

@mjclawar - #136 fixed the tests. Would you mind rebasing your branch off of master?

…arameters

…ramda for type checking; batch updates for setProps vs each field sequentially

… 81-location-url-parameters

mjclawar · 2018-01-02T21:47:22Z

Rebased.

chriddyp · 2018-01-02T21:55:06Z

Tests passed! 🎉 Thank you @mjclawar , this was a great PR 🍻

mjclawar · 2018-01-04T02:42:20Z

@chriddyp do you have an expected timeline for getting this published as the next version/how do you typically approach publishing for the dash packages? Thanks!

chriddyp · 2018-01-04T03:33:30Z

Hey @mjclawar - Thanks for the reminder! I just published v0.15.4 now

mjclawar added 10 commits December 20, 2017 18:48

Update gitignore for idea, Location.react.js and add package-lock.json

e65af17

Merge remote-tracking branch 'upstream/master' into 81-location-url-p…

14dba83

…arameters

Update package.json and package-lock.json from upstream

45013d5

Handle hash, href, pathname and search collectively

762bfc7

support Python3 import for urlparse

e7a5607

Functioning tests with Location

9cfae7a

Add snapshot test for location-link

18afd51

Add tests for "a" HTML tags and handle undefined search and hash values

608feac

Update CHANGELOG and bump version to 0.15.4

97768c5

0.15.4

c3d2ea3

mjclawar mentioned this pull request Dec 21, 2017

url parameters are not available in dcc.Location #81

Closed

chriddyp suggested changes Dec 21, 2017

View reviewed changes

Code style changes; moved waiter to utils (takes a waiter_func); use …

b196826

…ramda for type checking; batch updates for setProps vs each field sequentially

mjclawar mentioned this pull request Dec 22, 2017

Update dcc.Link API to better match window.location #132

Open

uncomment non-link/non-location tests

bee0b0e

chriddyp mentioned this pull request Jan 2, 2018

lock down dependencies #136

Merged

mjclawar added 6 commits January 2, 2018 16:45

Merge remote-tracking branch 'upstream/master' into 81-location-url-p…

593891d

…arameters

Update gitignore for idea, Location.react.js and add package-lock.json

eab3889

Update package.json and package-lock.json from upstream

ad98d7a

Handle hash, href, pathname and search collectively

59a4231

support Python3 import for urlparse

fe5741f

Functioning tests with Location

4833a32

mjclawar added 7 commits January 2, 2018 16:45

Add snapshot test for location-link

a2a15a9

Add tests for "a" HTML tags and handle undefined search and hash values

70b59ac

Update CHANGELOG and bump version to 0.15.4

9b28dad

0.15.4

d68f445

Code style changes; moved waiter to utils (takes a waiter_func); use …

6bb808f

…ramda for type checking; batch updates for setProps vs each field sequentially

uncomment non-link/non-location tests

f612d9f

Merge remote-tracking branch 'origin/81-location-url-parameters' into…

7d33972

… 81-location-url-parameters

chriddyp approved these changes Jan 2, 2018

View reviewed changes

chriddyp merged commit 4e54a67 into plotly:master Jan 2, 2018

mjclawar deleted the 81-location-url-parameters branch January 3, 2018 16:04

chriddyp mentioned this pull request Jan 4, 2018

Save state in url plotly/dash#188

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

81 location url parameters #131

81 location url parameters #131

mjclawar commented Dec 21, 2017 •

edited

Loading

chriddyp left a comment

chriddyp Dec 21, 2017 •

edited

Loading

chriddyp Dec 21, 2017

chriddyp Dec 21, 2017

chriddyp Dec 21, 2017

chriddyp Dec 21, 2017

mjclawar Dec 22, 2017

chriddyp Dec 21, 2017

chriddyp Dec 21, 2017

chriddyp Dec 21, 2017

chriddyp Dec 21, 2017

chriddyp Dec 21, 2017

chriddyp commented Dec 21, 2017

chriddyp commented Dec 22, 2017

mjclawar commented Dec 23, 2017

mjclawar commented Dec 26, 2017

chriddyp commented Jan 2, 2018

chriddyp commented Jan 2, 2018

mjclawar commented Jan 2, 2018

chriddyp commented Jan 2, 2018

mjclawar commented Jan 2, 2018

chriddyp commented Jan 2, 2018

mjclawar commented Jan 4, 2018

chriddyp commented Jan 4, 2018

81 location url parameters #131

81 location url parameters #131

Conversation

mjclawar commented Dec 21, 2017 • edited Loading

Description

Implementation

Tests added

chriddyp left a comment

Choose a reason for hiding this comment

chriddyp Dec 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chriddyp commented Dec 21, 2017

chriddyp commented Dec 22, 2017

mjclawar commented Dec 23, 2017

mjclawar commented Dec 26, 2017

chriddyp commented Jan 2, 2018

chriddyp commented Jan 2, 2018

mjclawar commented Jan 2, 2018

chriddyp commented Jan 2, 2018

mjclawar commented Jan 2, 2018

chriddyp commented Jan 2, 2018

mjclawar commented Jan 4, 2018

chriddyp commented Jan 4, 2018

mjclawar commented Dec 21, 2017 •

edited

Loading

chriddyp Dec 21, 2017 •

edited

Loading