-
-
Notifications
You must be signed in to change notification settings - Fork 41
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
Add core elements of Dash API, support generated components #18
Conversation
Enable Dash.jl integration tests and add startup message
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@waralex This is great! A lot of good features in this PR. The support for generated components is really nice to see, it's going to make supporting Dash.jl possible once we've merged the generator PR. I can see support for
- components generated prior to runtime
- asset serving
- asset fingerprinting
- asynchronous loading of components
- layout as component, tree of components, or functions returning components
- validation for index page and layout
- dev tools UI arguments
- querying environment variables (important for deployed apps)
I've made comments in-line, but one thing I noticed is that there are very few comments in the code. I would suggest adding them for any methods/functions whose purpose is not immediately obvious, and which do not exist in R or Python. In addition, there is what appears to be a new feature user-facing element in callback!
called pass_changed_props
; we should probably discuss since it appears to be a deviation from the Dash API. I recognize that this is a legacy item from Dashboards.jl.
Now that we have integration test support for Dash.jl, we will want to add a few tests before merging this PR. I'll recommend some initial tests we can convert from Python/R, but feel free to add others if you think they are helpful. It's really great to see this moving ahead, thanks for taking this on, and investing the energy in refactoring to bring the interface and feature set closer to parity with the existing versions of Dash.
Let's try to condense the quantity of new features in subsequent PRs if we can, this one (understandably) contains quite a bit, but it will be a little easier to review/revise if we try not to do too much in a single shot. 😸
Related to integration tests: we would like to add tests for basic functionality, which probably eventually includes
I can help with these, should help ensure stability of the code going forward, particularly as we near a release with all the new functionality you've worked hard to provide. |
… generated_components
@waralex I went ahead and resolved nearly all of the outstanding items, and added a test for meta tags (since I had added one for R already). I think this PR is basically ready to go once we've figured out what to do about #18 (comment). Nice work! 🙌 |
Co-authored-by: Ryan Patrick Kyle <[email protected]>
… generated_components
Looks OK; I think we're ready to merge this into |
Cache-Control
andETag
supportlayout
as a functionlayout
andindex_string
DevTools
structure andenable_dev_tools(app;debug, ....)
method for setting DevTools. Arguments are the same as in python version. Also that arguments added torun_server
functiondash()
withlayout_maker
is removedNot all docstrings is currently valid. I suggest updating all the documentation when we are ready for release to the master
For testing: activate pkg mode in REPL (press
]
in REPL mode)Remove existing Dash package:
Install Dash from this branch:
Install generated components packages from my forks:
Try to run simple server: