-
Notifications
You must be signed in to change notification settings - Fork 151
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
Enable dash datatable #114
Conversation
Co-authored-by: Li Nguyen <[email protected]> Signed-off-by: Maximilian Schulz <[email protected]>
Co-authored-by: Li Nguyen <[email protected]> Signed-off-by: Maximilian Schulz <[email protected]>
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.
This is fantastic work Max 🚀. I just left one question below.
After reverting app.py
, dashboard.yaml
, pyproject.toml
changes and after adding changelog file, I think the PR will be ready for merging.
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.
Very happy to approve this subject to some minor suggestions and reverting the changes to the demo app or whatever your intention is. I haven't tried out the latest incarnation of the code myself but looks like others have done so and checked the styling etc. 👍
Really amazing work overall - I know this wasn't easy and had lots of iteration but I think it turned out great! Definitely one for the new Highlights section in our changelog ✨ Very nice work on the docs and tests also 👍
In future let's try to avoid such big PRs with 100s of comments, which I know is mainly my fault for leaving so many comments and digressions and asking things to be reworked 😅 Sometimes it's hard to avoid these things or they just happen by accident I know, and I'm as guilty as anyone here, but let's be mindful of it next time and maybe work through it in a different way that's more manageable for everyone 🙂
Dashboards in my tests can't start with this error:
|
Description
vm.Table
model, alongside the shipped standard table functionvizro.charts.tables.dash_data_table
Further refactorings include:
ModelID
Things to be added once implementation is agreed upon:
EDIT
Main open questions - may not all need to be adressed in this PR:
bind_partials
- see Enable dash datatable #114 (comment) (fixed in other PR)dash_data_table
and correct name thereof - see Enable dash datatable #114 (comment)EDIT 2
PR almost finished, tasks left to do:
ModelID
!important
Screenshot
Checklist
Enable feature XXX ([#1](https://github.com/mckinsey/vizro/pull/1))
(if applicable)Types of changes
Notice
I acknowledge and agree that, by checking this box and clicking "Submit Pull Request":