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

[WIP] Dash component system support #490

Closed
wants to merge 14 commits into from
Closed

Conversation

T4rk1n
Copy link
Contributor

@T4rk1n T4rk1n commented Mar 18, 2019

No description provided.

@T4rk1n T4rk1n force-pushed the dash-component-system branch from 6ffdb52 to 0e5454b Compare March 20, 2019 17:52
@T4rk1n T4rk1n force-pushed the dash-component-system branch from 1ee69d1 to dd30118 Compare April 2, 2019 19:30
def __init__(
self,
id=UNDEFINED,
options="[]",
Copy link
Collaborator

Choose a reason for hiding this comment

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

@T4rk1n is it intentional that these empty objects are quoted ("[]", "{}") here and in ComponentProp?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, default props generation needs more testing.

@T4rk1n T4rk1n force-pushed the dash-component-system branch from a18e182 to 9cd1826 Compare April 8, 2019 16:50
for more info.(default={ staticPlot: false,
editable: false, edits: {
annotationPosition: false, annotationTail:
false, annotationText: false,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hey re: #514 I'm thinking of removing these defaults, just using config: {}, as well as writing some sort of test that the PropTypes.shape we give matches the available options in the current version of plotly.js. But in this PR the default seems to be the only info about what's allowed inside config that makes it into the Python docs. How do you think we should handle this?

  • bring the shape into this docstring programmatically?
  • require that this shape be described explicitly in the docstring for graphPropTypes.config (and then I'd write a test to verify that this is kept up-to-date)?
  • something else?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The default in the docstring come from Graph.defaultProps, if you change it to {} they will be removed.

bring the shape into this docstring programmatically?

You cannot do that on at runtime with react-docgen as nothing is evaluated just parsed. It has to be in the source. Maybe a script that fetch and format and you insert the output in the docstring, I'd imagine you'd only have to do that when you update plotly.js.

require that this shape be described explicitly in the docstring for graphPropTypes.config (and then I'd write a test to verify that this is kept up-to-date)?

I like that.

something else?

Front-end validations and keep as is with the default {}?

Copy link
Contributor Author

@T4rk1n T4rk1n Apr 8, 2019

Choose a reason for hiding this comment

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

Come to think of it, the mypy validation for shape is shallow and not key based, it might be worthwhile to format the PropType definition in the docstring.

Copy link
Collaborator

Choose a reason for hiding this comment

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

OK cool - one way or another we can handle this in documentation. For now I'm going to take the defaults out for #514

@byronz byronz changed the base branch from master to dev August 14, 2019 20:58
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.

3 participants