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

Remove graph config innards from defaultProps #515

Merged
merged 7 commits into from Apr 9, 2019

Conversation

Projects
None yet
2 participants
@alexcjohnson
Copy link
Contributor

commented Apr 9, 2019

Addresses most of #514 - doesn't include a test (yet?), but removes the config innards and adds the missing pieces to propTypes.

alexcjohnson and others added some commits Apr 9, 2019

remove Graph.defaultProps.config innards
and add missing config items to propTypes
Marc-André Rivet
@Marc-Andre-Rivet

This comment has been minimized.

@Marc-Andre-Rivet Marc-Andre-Rivet self-requested a review Apr 9, 2019

@Marc-Andre-Rivet

This comment has been minimized.

Copy link
Contributor

commented Apr 9, 2019

The renderer also uses the graph for a few tests, nothing deep, but might be worth taking some time to create a renderer branch pointing to this dcc branch to increase sanity/coverage.

@@ -557,7 +557,7 @@ const graphPropTypes = {
* Extra resolution to give the file after
* rendering it with the given width and height
*/
scale: PropTypes.number
scale: PropTypes.number,

This comment has been minimized.

Copy link
@alexcjohnson

alexcjohnson Apr 9, 2019

Author Contributor

Ah thanks. I had done that locally but hadn't pushed it yet as I'm adding a couple of tests.

alexcjohnson added some commits Apr 9, 2019

test some stuff about plotly.js vs Graph
also remove an old plotly.js bundle
@@ -1193,7 +1193,7 @@ def on_click(n_clicks):
def check_plotlyjs(self):
# find plotly.js files in the dist folder, check that there's only one
all_dist = os.listdir(dcc.__path__[0])
js_re = '^plotly-(.*)\.min\.js$'
js_re = r'^plotly-(.*)\.min\.js$'

This comment has been minimized.

Copy link
@alexcjohnson

alexcjohnson Apr 9, 2019

Author Contributor

oddly I can't get this error to appear locally... 😐

)
with open(os.path.join(dcc.__path__[0], 'metadata.json')) as meta:
graph_meta = json.load(meta)['src/components/Graph.react.js']
config_prop_shape = graph_meta['props']['config']['type']['value']

This comment has been minimized.

Copy link
@alexcjohnson

alexcjohnson Apr 9, 2019

Author Contributor

kind of ugly to have to use metadata.json for this... I was hoping I could get this directly from the Graph component, but PropTypes seems to turn everything into a validation function, I don't see a way to get the input params back out.

if item_name in ignored_config:
continue

self.assertIn(item_name, props)

This comment has been minimized.

Copy link
@alexcjohnson

alexcjohnson Apr 9, 2019

Author Contributor

Currently not testing anything about the props, just that the right keys exist. We could enhance this later to verify that their types match what's in the schema. Note that we could have put back in the deep defaults and used a similar framework to test that they match the schema, but I don't think that's a good idea. We'd still have the problem of a single override dropping the defaults - in principle picking up the right defaults again from plotly.js, but there could be subtle problems with this, or perhaps a user overrides the plotly.js version... I'd rather keep the looser coupling.

js_re = r'^plotly-(.*)\.min\.js$'
plotlyjs_dist = [fn for fn in all_dist if re.match(js_re, fn)]

self.assertEqual(len(plotlyjs_dist), 1)

This comment has been minimized.

Copy link
@alexcjohnson

alexcjohnson Apr 9, 2019

Author Contributor

I don't know how we ended up with 2 copies of plotly.js on master, 1.44.1 and 1.45.0... the associated PRs all seem to swap one file for another, maybe a mis-merge somewhere. But anyway this test will prevent that in the future.

/**
* add the plotly logo on the end of the mode bar
*/
displaylogo: PropTypes.bool,

/**
* add the plotly logo even with no modebar

This comment has been minimized.

Copy link
@Marc-Andre-Rivet

Marc-Andre-Rivet Apr 9, 2019

Contributor
Suggested change
* add the plotly logo even with no modebar
* Add the plotly logo even with no modebar

This comment has been minimized.

Copy link
@alexcjohnson

alexcjohnson Apr 9, 2019

Author Contributor

Ah right, a lot of the older docstrings didn't bother capitalizing so I tried to follow that, but then some of the newer ones do... I should just go through and capitalize them all. Thanks for pointing this out.

@@ -491,6 +501,13 @@ const graphPropTypes = {
*/
displayModeBar: PropTypes.oneOf([true, false, 'hover']),

/**
* should we include a modebar button to send this data to a

This comment has been minimized.

Copy link
@Marc-Andre-Rivet

Marc-Andre-Rivet Apr 9, 2019

Contributor
Suggested change
* should we include a modebar button to send this data to a
* Should we include a modebar button to send this data to a
@@ -374,6 +374,11 @@ const graphPropTypes = {
*/
staticPlot: PropTypes.bool,

/**
* base URL for a Plotly cloud instance, if `showSendToCloud` is enabled

This comment has been minimized.

Copy link
@Marc-Andre-Rivet

Marc-Andre-Rivet Apr 9, 2019

Contributor
Suggested change
* base URL for a Plotly cloud instance, if `showSendToCloud` is enabled
* Base URL for a Plotly cloud instance, if `showSendToCloud` is enabled
@Marc-Andre-Rivet
Copy link
Contributor

left a comment

This looks fine to me 💃 Nice test!

@alexcjohnson alexcjohnson merged commit 179600e into master Apr 9, 2019

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

@alexcjohnson alexcjohnson deleted the no-graph-config-dflts branch Apr 9, 2019

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.