-
Notifications
You must be signed in to change notification settings - Fork 142
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
[Bug] Fix reloading issues of the AG Grid #446
Conversation
for more information, see https://pre-commit.ci
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.
@maxschulz-COL thank you for all your work on this PR. I know how hard it was to test all the cases and options and to find what was causing the persistence issues.
By returning the original data_frame
fromfigure.build
and with other changes (like the default dash_ag_grid configuration) you really solved all the mentioned issues.
I tried to find any inconsistency by altering arguments such as: persistence, persistence_type, persisted_props, columnSize and defaultColDef and found that the problem with the filterModel
disappeared completely.
However columnSize
doesn't work if configured in combination withsomething like defaultColDef={"flex": 1, "minWidth": 200},
. This is really an edge case where the user can expect that the column size is auto-sized but that the width is never lower than 200. Also, if columnSize="autoSize"
is set in the combination with persisted_props=["columnSize"]
, "autoSize" doesn't work at all. This seems strange to me since, columnSize
and filerModel
should behave same since they are both dag.AgGrid properties.
I would go with this solution (hence the approval from my side), but we should also create a new ticket to optimise the page load process since it doesn't seem like a perfect solution to return the original data_frame from figure.build
and to overwrite it immediately afterwards with on-page-load
.
FYI @petar-qb since you were away at the time, just for reference see my comment here: #439 (review). |
@petar-qb If I understand it correctly, you are essentially referring to user created If this is what you also meant, then let's go with the proposed solution indeed. Any user question we can refer to the Dash AG Grid, because we are not altering that implementation in any way and thus the answer can only lie there. |
I totally agree, and as I said: I would go with this solution (hence the approval from my side) 😄 |
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
Description
This PR closes the following issues:
After some investigation, it looks like there is some confirmed buggy behaviour:
columnSize="autoSize"
will not see his results consistently)The identified two underlying reasons are:
_dash_ag_grid
functions that concern columns settings that could potentially conflict with user chosen values such ascolumnSize="autoSize"
It has to be noted that this behaviour is not always easy to investigate, as there are many moving pieces etc. This PR presents to options for investigation:
app.py
- Vizro app setup to investigate different options.app2.py
(will be deleted later) Pure Dash App to test what "bugs" carry overProposed solution
_dash_ag_grid
Those two changes fix all observed problems for me. This is the current status of the code, and it would be good if reviewers could test out if the see the same with the various settings possible.
How to review
Run
hatch run example
and test the various options proposed inapp.py
. Particular focus should be on:app2.py
and the additional files underpages
is just there in case we want to try out things in a pure Dash app. I will delete once we agree on a solution.Screenshot
Notice
I acknowledge and agree that, by checking this box and clicking "Submit Pull Request":