PablocFonseca / streamlit-aggrid

Implementation of Ag-Grid component for Streamlit
https://pypi.org/project/streamlit-aggrid/
MIT License
1.05k stars 199 forks source link

Row selection events not reported #259

Closed mmf431 closed 5 months ago

mmf431 commented 6 months ago

Row selection events are not reported. I took the examples that one finds in the web. They do not report what they are supposed to do.

amitmadahar commented 6 months ago

I second that. I think that is the reason for row selection not working in the latest release either. It works ok in v0.3.4.

if grid_response['selected_rows']: currentSelection = grid_response['selected_rows'][0] st.session_state.currentSelectedFile = currentSelection

grid_response['selected_rows'] does not contain the selected row dataframe.

burkelawlor commented 6 months ago

I am also encountering an error in the latest version with row select. Specifically, when trying to call the selected row from the grid response, I am getting AttributeError: 'list' object has no attribute 'items'.

Can confirm that this error does not happen in v0.3.4.

amitmadahar commented 6 months ago

I am also encountering an error in the latest version with row select. Specifically, when trying to call the selected row from the grid response, I am getting AttributeError: 'list' object has no attribute 'items'.

Can confirm that this error does not happen in v0.3.4.

yes its works in v0.3.4

robingie commented 6 months ago

I can confirm the behaviour as well. It seems like the grid_response.selected_rows doesn't get fully populated. The property grid_response_rows_id on the other hand has the selected rows id indexes. Is the above behaviour intended? It currently breaks the existing implementation, therefore I have stay at version 0.3.3

I hope this gets fixed soon.

PablocFonseca commented 6 months ago

On v.1.0.0. I changed the response object, so items are accessed via dot notation instead of brackets. I'm moving response processment from Javascript to Python side, to improve performance. It broke backwards compatibility. I added some code on v.1.0.1 to enable backward compatibility, by returning with brackets notation for some props.

amitmadahar commented 6 months ago

@PablocFonseca - thanks for looking into this so quickly. You are correct be the dot notation, however, the returned response does not contain the selected row/s dataframe like it use to. Of course selection.selected_ids gives the address of the selected row that can be used to get to selected df values as a workaround but the backward compatibility still breaks. The problem I faced was not the dot notation but the lack of response dataframe.

mkleinbort-ic commented 6 months ago

I'm having an issue where the table is ignoring rowSelection = 'single'. The return value is still the single selected row, but the table "adds" a selected row without de-selecting the previous row (as far as visuals is concerned)

I'm using checkbox selection

robingie commented 5 months ago

@PablocFonseca - After the recent update to 1.0.2 the data gets populated to selection dataframe. - Great!

Therefore, after implementing it with the rot notation I ran into another issue. The response data has different columns based on whether at least one row was selected. In case, no row was selected, the output is just an empty dataframe. This circumstance makes it necessary to always run a check like if gridresponse.selected_rows.empty then pass else gridresponse.selected_rows["Column1"].to_list()

Maybe it would be beneficial to keep the dataframe structure identically to avoid such extra checks.

PablocFonseca commented 5 months ago

@robingie I agree it makes sense.

So, do you think its better to return a DF with columns and no rows, or None?

amitmadahar commented 5 months ago

@robingie I agree it makes sense.

So, do you think its better to return a DF with columns and no rows, or None?

I would vote for None as No Selection = None and makes it easy on downstream logic.

robingie commented 5 months ago

@PablocFonseca As for my current use cases, I would prefer having an empty data frame with all the columns available. Most of the time I have to extract some ids from an id column or another reference and store it in a list for further processing. An empty list will cause no issue when you iterate over it.

Alternatively, None as a return value requires to catch it separately and can be quite bothersome depending on how much processing you do with the response.

Usually, I have to iterate over the selected records. With the new data frame some extra logic needs to be added over it. But I guess it's fine so that we stay consistent. On the other hand, if you just depend on a single allowed selection return None might be more feasible.

amitmadahar commented 5 months ago

But breaks the backward compatibility for 10s others which should make this an unpopular choice and unwanted hassle to discover that the selection return logic has changed. Just my penny worth!

PablocFonseca commented 5 months ago

I believe the previous behaviour was to return an empty list, which would behave similar when testing for None. I'll double check previous version returns and try to keep backward compatibility. I'll also add convenience methods to AgGridReturn, so we can try to have best of both solutions.

PablocFonseca commented 5 months ago

I updated the data and selected_data behaviour on v 1.0.3-2: They have identical return formats, except one return only selected rows and other all data. I also included a property to deal with rowGroups

Live example here https://staggrid-examples.streamlit.app/Selecting_data