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

Selected rows fixes #283

Closed
wants to merge 22 commits into from
Closed

Selected rows fixes #283

wants to merge 22 commits into from

Conversation

BSd3v
Copy link
Collaborator

@BSd3v BSd3v commented Apr 9, 2024

  • adding timeouts to keep async processes together

@brifordwylie
Copy link

brifordwylie commented Apr 10, 2024

this is looking promising. I tried to check out the branch and test it out (even before merge) just to see if my test case would work. I got an error when importing the dash_ag_grid module (might be pilot error). Here's the reproduction steps...

git clone git@github.com:BSd3v/dash-ag-grid-real.git dash-ag-grid-beta
cd dash-ag-grid-beta
<commit = commit 7acd81ea (HEAD -> selectedRows-fixes, origin/selectedRows-fixes)>
npm i npm run build
pip install -e .

Then just try importing the package..

ipython
In [1]: import dash_ag_grid
---------------------------------------------------------------------------
ModuleNotFoundError                       Traceback (most recent call last)
Cell In[1], line 1
----> 1 import dash_ag_grid

File ~/work/dash-ag-grid-beta/dash_ag_grid/__init__.py:10
      7 import dash as _dash
      9 # noinspection PyUnresolvedReferences
---> 10 from ._imports_ import *
     11 from ._imports_ import __all__
     13 if not hasattr(_dash, '__plotly_dash') and not hasattr(_dash, 'development'):

ModuleNotFoundError: No module named 'dash_ag_grid._imports_'

@BSd3v
Copy link
Collaborator Author

BSd3v commented Apr 10, 2024

To run from a branch you need to build the package.

npm i
npm run build

Doing them on the same line without an & i think won't work.

@brifordwylie
Copy link

brifordwylie commented Apr 10, 2024

Ah, silly me... I think the Contributing.md shows them smooshed together. Anyway running my test code against your branch looks good... I can callback/update the table with "columnDefs", "rowData", "selectedRows" and it all looks like it working properly. Thanks a bunch for the fix. Looking forward to new PyPI version when you get a chance :)

(update) The only small wrinkle is that this callback gets called twice, so there seems to be a 'bounce' that happens... it gets called once as a result of the callback that outputs the 'selectedRows" and then it appears to get called again from the widget itself...

@app.callback(Output("selected-row-info", "children"), Input("my-table", "selectedRows"))
def update_selected_row_info(selected_rows):
    selected_row_info = f"Selected Rows: {selected_rows}"
    print(selected_row_info)
    return selected_row_info

Comment on lines +38 to +39
pip uninstall -y pytest
pip install 'pytest<8.1.0'
Copy link
Member

Choose a reason for hiding this comment

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

Why is there an uninstall step here? Also, why pin at <8.1.0?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Pytest has an issue with something and was failing. Reverting back to a lower version solved the issue.

this.reference = React.createRef();
this.pendingChanges = null;
this.dataUpdates = false;
Copy link
Member

Choose a reason for hiding this comment

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

What are the new pauseSelections/dataUpdates props for?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

pauseSelections isnt new, its to keep systematic adjustments from triggering the onSelectionChanged, as far as dataUpdates its to be used in the event of rowData pushed from the server, this way it can ignore selection events. As this clears out the selectedRows if there is no data there:

rowData became a async process, so both were triggered at the same time. But because the rowData was updating, when the selectedRows were trying to be applied the grid was essentially empty.

Copy link
Member

@ndrezn ndrezn Apr 16, 2024

Choose a reason for hiding this comment

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

Got it. Just wondering, is there an open ticket that describes these issues? #274 looks like it's describing this..?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, this is linked with the two issues already. :)

Copy link
Member

Choose a reason for hiding this comment

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

aha... yep catching up to you hah

- browser-tools/install-browser-tools:
chrome-version: 116.0.5845.96
firefox-version: 116.0.3
- browser-tools/install-chrome
Copy link
Member

Choose a reason for hiding this comment

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

These diffs are all coming from #281 right? So we'll need to merge #281 first, rebase, then merge in these changes..?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes. That is correct.

@@ -95,3 +97,75 @@ def selected(n):
dash_duo.wait_for_text_to_equal(
"#selections-single-output", "You selected athlete: Natalie Coughlin"
)

def test_sr2_selected_rows_rowdata(dash_duo):
Copy link
Member

@ndrezn ndrezn Apr 16, 2024

Choose a reason for hiding this comment

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

Reviewing the test cases in #274:

We're covering:

  • Cannot output rowData and selectedRows in the same callback

But confirming this covers:

  • Inconsistent data type when selectedRows is an input depending how selectedRows was specified
  • When using rowIds and checkbox selection, the checkboxes are never visibly selected

Could you add a comment to this test describing these cases as well so it's clear what it's testing for?

This example is much clearer and more concise overall than the originals, kudos for the MRE!

Comment on lines +137 to +138
selected_rows = df.head(1).to_dict("records")
return column_defs, row_data, selected_rows
Copy link
Member

Choose a reason for hiding this comment

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

Nice test for selecting the first row for #282 👍

Copy link
Member

@ndrezn ndrezn left a comment

Choose a reason for hiding this comment

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

💃 Please rebase from main and then I can merge! Nice change and great test case!

@ndrezn ndrezn mentioned this pull request Apr 22, 2024
@BSd3v BSd3v closed this Apr 22, 2024
@BSd3v
Copy link
Collaborator Author

BSd3v commented Apr 22, 2024

Closed as this had rebasing issues, replaced by:

#290

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.

3 participants