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

Track data changes #47

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

AdrianIssott
Copy link

About

This PR fixes #10 and as a bonus side-affect makes all of the previously read-only properties of the dash_pivottable.PivotTable writeable.

Description of changes

  • The first commit fixes some pre-existing pylint issues
  • The second commit updates usage.py to demonstrate that changes to data are not being picked up by the pivot table.
  • The third commit fixes that by using props values and not writing to state as inspired by the following console warning emitted when loading usage.py:
Warning: PivotTable: It is not recommended to assign props directly to state because updates to props won't be reflected in state. In most cases, it is better to use props directly.
    in PivotTable (created by CheckedComponent)
    in CheckedComponent (created by BaseTreeContainer)
    in ComponentErrorBoundary (created by BaseTreeContainer)
    in BaseTreeContainer (created by Context.Consumer)
    in Unknown (created by BaseTreeContainer)
    in div (created by u)
    in u (created by CheckedComponent)
    in CheckedComponent (created by BaseTreeContainer)
    in ComponentErrorBoundary (created by BaseTreeContainer)
    in BaseTreeContainer (created by Context.Consumer)
    in Unknown (created by UnconnectedContainer)
    in div (created by UnconnectedGlobalErrorContainer)
    in div (created by GlobalErrorOverlay)
    in div (created by GlobalErrorOverlay)
    in GlobalErrorOverlay (created by DebugMenu)
    in div (created by DebugMenu)
    in DebugMenu (created by UnconnectedGlobalErrorContainer)
    in div (created by UnconnectedGlobalErrorContainer)
    in UnconnectedGlobalErrorContainer (created by withRadiumContexts(UnconnectedGlobalErrorContainer))
    in withRadiumContexts(UnconnectedGlobalErrorContainer) (created by ConnectFunction)
    in ConnectFunction (created by UnconnectedContainer)
    in UnconnectedContainer (created by ConnectFunction)
    in ConnectFunction (created by UnconnectedAppContainer)
    in UnconnectedAppContainer (created by ConnectFunction)
    in ConnectFunction (created by AppProvider)
    in Provider (created by AppProvider)
    in AppProvider
  • The fourth commit the readme to say that changes to all the previous read-only only pivottable values can now be written to whilst also updating usage.py to demonstrate that is indeed now possible.

Pre-Merge checklist

  • The project was correctly built with npm run build:all.
  • If there was any conflict, it was solved correctly.
  • All changes were documented in CHANGELOG.md.
  • All tests on CircleCI have passed.
  • All Percy visual changes have been approved.
  • Two people have 💃'd the pull request. You can be one of these people if you are a Dash Pivottable core contributor.

Reference Issues

Fixes #10

pylint 2.14.4
astroid 2.11.6
Python 3.8.2 (tags/v3.8.2:7b3ab59, Feb 25 2020, 23:03:10) [MSC v.1916 64 bit (AMD64)]
…ndererName, rows, rowOrder, vals and valueFilter can be updated and adding buttons to usage.py to demonstrate that
@AdrianIssott
Copy link
Author

The build is failing due to pylint failing to run (e.g. here) which is due to this restriction on asteriod.

If I remove that line then pylint locally runs fine.

@jborman-exos
Copy link

@alexcjohnson @nicolaskruchten - Is there anything outstanding with this PR? Considering that issue #10 was first opened in 2019 and this is (imo) a major pain-point in working with this component (which is otherwise a great add to the Dash toolkit), it would be great if we could prioritize getting this merged to master and released.

@AdrianIssott
Copy link
Author

@chriddyp any chance you could help move this PR along?

@jaapaap79
Copy link

Any resolution to get this merged in?

@candas-unal-work
Copy link

Hey@AdrianIssott,

I applied your changes for my pivot table but I filter my columns or rows, the valueFilter argument is still empty on my callbacks. Does it work for you or did I understand its functionality wrong?

@AdrianIssott
Copy link
Author

AdrianIssott commented Jan 5, 2023

I applied your changes for my pivot table but I filter my columns or rows, the valueFilter argument is still empty on my callbacks. Does it work for you or did I understand its functionality wrong?

@candas-unal-work, it does work for me. I updated usage.py to demonstrate that so I suggest you check if that works for you too or not.

@candas-unal-work
Copy link

candas-unal-work commented Jan 6, 2023

@AdrianIssott OK this is interesting:

When I tried the usage.py with your changes it worked properly, and I've been trying to understand what I was missing in my app. I finally solved the problem by declaring the pivot table initially with an empty valueFilter as valueFilter=dict(). Now I can retrieve the filtered values in columns and rows, and if I don't pass an empty dictionary, I get an empty list.

Thank you.

@AdrianIssott
Copy link
Author

Well that's not great and suggests an improvement is needed on this PR. However, there's little point in me doing that given it's totally stalled

@candas-unal-work
Copy link

I understand. But thank you again for the PR, it solved my many problems @AdrianIssott .

@jaapaap79
Copy link

How could this project be revived?

@candas-unal-work
Copy link

I think pivot table is a great tool, has a lot of potentials but the people at plotly are not as interested, or so it seems. @alexcjohnson @nicolaskruchten .

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

Successfully merging this pull request may close these issues.

Pivottable doesn't get updated in successive callbacks
4 participants