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

Table filter interactions #119

Merged
merged 143 commits into from
Nov 2, 2023
Merged

Table filter interactions #119

merged 143 commits into from
Nov 2, 2023

Conversation

petar-qb
Copy link
Contributor

@petar-qb petar-qb commented Oct 19, 2023

Description

  • Filter interaction enabled from the vm.Table model when its figure argument represents the dash.Datatable object.

Potential issues/inconsistencies:

vm.Table as output (of filter, parameter, on_page_load, filter_interactions) works fine because we always modify (as the action's result) outer Div component which ID we know. There are some problems when vm.Table is the input (trigger or state) of the action:

  • Defining actions on the vm.Table object actually means defining a callback where the trigger is underlying table component fully created by user. It means that we can't set underlying table ID and we need to "force" dashboard users to add the ID if the table is action's trigger.
  • Our vm.Table will behave differently depending on is the underlying component dash.Datatable or AgGrid. e.g. different filter_interaction input properties are considered and different way of calculating filter_interaction's result.
  • Persistency of dash.Datatable "action_cell" and chart's "clickData" properties behave differently and we need to consider aligning them. e.g. you can unmark "active_cell" in different ways, and the only way to unmark chart's "clickData" is to refresh the page.

TODOs:

  • Implement unit tests.
  • Add table filter interaction example in docs
  • Introduce Figure in docs
  • Add table filter interaction example in app.py
  • Remove any AgGrid reference from project

Checklist

  • I have not referenced individuals, products or companies in any commits, directly or indirectly
  • I have not added data or restricted code in any commits, directly or indirectly
  • I have updated the docstring of any public function/class/model changed
  • I have added the PR number to the change description in the changelog fragment, e.g. Enable feature XXX ([#1](https://github.com/mckinsey/vizro/pull/1)) (if applicable)
  • I have added tests to cover my changes (if applicable)

Types of changes

  • Docs/refactoring (non-breaking change which improves codebase)
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Notice

  • I acknowledge and agree that, by checking this box and clicking "Submit Pull Request":

    • I submit this contribution under the Apache 2.0 license and represent that I am entitled to do so on behalf of myself, my employer, or relevant third parties, as applicable.
    • I certify that (a) this contribution is my original creation and / or (b) to the extent it is not my original creation, I am authorized to submit this contribution on behalf of the original creator(s) or their licensees.
    • I certify that the use of this contribution as authorized by the Apache 2.0 license does not violate the intellectual property rights of anyone else.

Copy link
Contributor

@huong-li-nguyen huong-li-nguyen left a comment

Choose a reason for hiding this comment

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

Looks good from my side - thanks for incorporating all the comments 👍 🚀

Would be great if you could add the additional table example and remove any reference to the AG grid in the requirements and code. Other than that, all good from my side 👍

@petar-qb
Copy link
Contributor Author

petar-qb commented Nov 1, 2023

There are few more things I'm currently working on (and are the part of the PR Description - ToDos section):

  • Add table filter interaction example in docs.
  • Introduce "Figure" in docs.
  • Add table filter interaction example in app.py.
  • Remove any reference to AgGrid in project.

In the meantime other parts of the PR are ready for review.

FYI: @maxschulz-COL, @huong-li-nguyen 😄

Copy link
Contributor

@maxschulz-COL maxschulz-COL left a comment

Choose a reason for hiding this comment

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

Thanks a lot! Have a couple more suggestions, but I think we are getting there 💪

Need to be careful so that tests don't become unreadable! :)

@maxschulz-COL
Copy link
Contributor

There are few more things I'm currently working on (and are the part of the PR Description - ToDos section):

  • Add table filter interaction example in docs.
  • Introduce "Figure" in docs.
  • Add table filter interaction example in app.py.
  • Remove any reference to AgGrid in project.

In the meantime other parts of the PR are ready for review.

FYI: @maxschulz-COL, @huong-li-nguyen 😄

You beat me to my comment :) Reviewed the test, but happy to take another look once the above is done!

Copy link
Contributor

@maxschulz-COL maxschulz-COL left a comment

Choose a reason for hiding this comment

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

LGTM now, but please still adress the last open issues

vizro-core/examples/default/app.py Show resolved Hide resolved
@petar-qb petar-qb merged commit acbba54 into main Nov 2, 2023
16 checks passed
@petar-qb petar-qb deleted the feat/table_filter_interaction branch November 2, 2023 15:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature Request 🤓 Issue contains a feature request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants