Skip to content

Dash components: Review checklist #420

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

Closed
VeraZab opened this issue Oct 5, 2018 · 10 comments
Closed

Dash components: Review checklist #420

VeraZab opened this issue Oct 5, 2018 · 10 comments

Comments

@VeraZab
Copy link

VeraZab commented Oct 5, 2018

Hey guys,
I was wondering if you'd find it useful to have a Dash app review checklist. I know that there's review apps set up for most dash repos now, and that's great.

But I'm thinking that, especially for new reviewers and makers of Dash components, it would be useful to have a few checkpoints to look at before saying a component is ready for release.

Example of things I'm thinking of:

  • Component props: making sure that for each one we've thought about whether dash should be able to set them, or whether its something that should only be dealt with in the front-end, or if both front-end and dash should be able to affect their value.
  • React doc strings, should all components have a basic set of info in Doc strings?
  • Should all components work with any set of external data input? Is it up to the component or user to transform data in an acceptable format.
  • Are all my assets properly included so that when I publish my component, the community will also have access?

That's just an example of questions I'm thinking of when looking at components, but I'm sure there's others we could think.

Also, if we did make such a checklist, where would it live?

@valentijnnieman
Copy link
Contributor

Yeah I think that's a good idea, it would certainly help in getting more users to make Dash components. I don't really know where it should live though. I'm thinking it should go somewhere in the component boilerplate, because that's what people will be using. Perhaps just in the readme of that repo?

@nicolaskruchten
Copy link
Contributor

I would add that if any of these checks can be automated (i.e. insufficient docs, assets in MANIFEST) then they should raise warnings during the build step, to help people do things right :)

@T4rk1n
Copy link
Contributor

T4rk1n commented Oct 5, 2018

There's a section about publishing your component in the boilerplate. Can add more info there. But I think the bulk of the information to build/publish components should be in dash-docs.

Should all components work with any set of external data input? Is it up to the component or user to transform data in an acceptable format.

I don't understand this part, you mean validating the props in the component ?

Are all my assets properly included so that when I publish my component, the community will also have access?

This definitely needs to be documented somewhere as you need to edit the __init__ file.

@VeraZab
Copy link
Author

VeraZab commented Oct 5, 2018

I don't understand this part, you mean validating the props in the component ?

I'm talking about when you're making your component, you might have tested only one end of the system (React), but you haven't tested that things work properly when updates happen from Python.

One example that comes to mind is if there's a prop that you're only taking into consideration in componentDidMount, it gets considered once in your front-end code, but them any subsequent updates coming from Dash won't change anything in your front-end.

Or if you're including some helpers in your front-end code, to help format the data your component takes, well, maybe it should be thought about as well from the Python end of things, should those helpers also exist on the Python side.. Just have a mental framework of what Dash -> React / React -> Dash / React < - > Dash interactions each prop enables.

Just having this as an item to think about and review before publishing could be helpful.

@T4rk1n
Copy link
Contributor

T4rk1n commented Oct 5, 2018

I would add that if any of these checks can be automated (i.e. insufficient docs, assets in MANIFEST) then they should raise warnings during the build step, to help people do things right :)

The docstring validation could be added to extract-meta script.
The assets validation could be a prepublish script.

@T4rk1n
Copy link
Contributor

T4rk1n commented Oct 5, 2018

I'm talking about when you're making your component, you might have tested only one end of the system (React), but you haven't tested that things work properly when updates happen from Python.

Personally, I only test my components in a full python dash environment, there's no point in testing them on react except that it may faster to write. There can be much differences in how dash handles the callbacks/updates.

With pytest-dash, I write the usage.py file and then load it to make a selenium integration test.

@VeraZab
Copy link
Author

VeraZab commented Oct 5, 2018

Personally, I only test my components in a full python dash environment, there's no point in testing them on react except that it may faster to write.

I think that may not be what a lot of newbies do :) especially if we tell them that the only thing you need in order to make a Dash component, is to write a React one, compatible with the Dash ecosystem. I think that the fact of writing mostly javascript for your component may make you forget about how you'll interact with this component from the Python end. And having a reminder to think about those interactions may be good.

@T4rk1n
Copy link
Contributor

T4rk1n commented Oct 5, 2018

Added missing docstring warnings to the cookiecutter.

plotly/dash-component-boilerplate@8d684d1

@T4rk1n
Copy link
Contributor

T4rk1n commented Oct 5, 2018

Added assets check warnings in the cookiecutter.

plotly/dash-component-boilerplate@0988a63

@gvwilson
Copy link
Contributor

Hi - we are tidying up stale issues and PRs in Plotly's public repositories so that we can focus on things that are most important to our community. If this issue is still a concern, please add a comment letting us know what recent version of our software you've checked it with so that I can reopen it and add it to our backlog. (Please note that we will give priority to reports that include a short reproducible example.) If you'd like to submit a PR, we'd be happy to prioritize a review, and if it's a request for tech support, please post in our community forum. Thank you - @gvwilson

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants