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

Propagate muti-cell paste event to Dash #261

Merged
merged 20 commits into from
Jan 31, 2024
Merged
Show file tree
Hide file tree
Changes from 16 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,11 @@ All notable changes to `dash-ag-grid` will be documented in this file.
This project adheres to [Semantic Versioning](https://semver.org/).
Links "DE#nnn" prior to version 2.0 point to the Dash Enterprise closed-source Dash AG Grid repo

## UNRELEASED

### Fixed
- [#262](https://github.com/plotly/dash-ag-grid/issues/262) Fixed all-but-one `cellValueChanged` events suppressed for multi-cell edits
Copy link
Collaborator

Choose a reason for hiding this comment

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

Rather than ### Fixed this should be a ### Changed with a very prominent note that it's a breaking change. So it's good that we're about to release a new major version for AG Grid v31 or this kind of change would not be allowed 😅 But it makes sense, I don't see any non-breaking way to support these events.

@BSd3v this makes me wonder, are there any other props that might have similar problems? If so now is the time to convert them into arrays too!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If backwards compatibility is a main concern, we could

  • Set the type of cellValueChanged to union of single/list of events
  • Add a new prop that represents CELL_VALUE_CHANGED_DEBOUNCE_MS , which defaults to -1, in which case we fallback to the old behavior, thus returning a single element. Setting the value it to 0 (or higher) will enable the new behavior, thus returning a list

My initial implementation was actually along these lines, but @BSd3v suggested to target a simpler implementation (the one in this PR) in favor of backwards compatibility since a major release is in the making.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yep, I agree the simpler API is more important than backward compatibility, given the upcoming major release and the fact it would be annoying to use the prop if sometimes it’s a list and sometimes not. Just make sure the changelog makes the breaking change clear.

Copy link
Collaborator

Choose a reason for hiding this comment

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

So far, I haven't found anything that operates this way.

The team has also been working on the v31 updates for the docs, and those types of things haven't popped up that I am aware of.

The one thing I don't know about is the grid's state (new feature of the grid).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have updated the comment, indicating that this is a change. Should I note explicitly that the change is breaking (I am not sure how breaking changes are documented in this project - I didn't see any mentioned of breaking changes previously in the changelog)?

Copy link
Collaborator

Choose a reason for hiding this comment

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

This would be the first since switching to version 2 from 1.


## [2.4.0] - 2023-10-17

### Added
Expand Down
74 changes: 38 additions & 36 deletions src/lib/components/AgGrid.react.js
Original file line number Diff line number Diff line change
Expand Up @@ -682,42 +682,44 @@ DashAgGrid.propTypes = {
/**
* Value has changed after editing.
*/
cellValueChanged: PropTypes.shape({
/**
* rowIndex, typically a row number
*/
rowIndex: PropTypes.number,

/**
* Row Id from the grid, this could be a number automatically, or set via getRowId
*/
rowId: PropTypes.any,

/**
* data, data object from the row
*/
data: PropTypes.object,

/**
* old value of the cell
*/
oldValue: PropTypes.any,

/**
* new value of the cell
*/
newValue: PropTypes.any,

/**
* column where the cell was changed
*/
colId: PropTypes.any,

/**
* Timestamp of when the event was fired
*/
timestamp: PropTypes.any,
}),
cellValueChanged: PropTypes.arrayOf(
PropTypes.shape({
/**
* rowIndex, typically a row number
*/
rowIndex: PropTypes.number,

/**
* Row Id from the grid, this could be a number automatically, or set via getRowId
*/
rowId: PropTypes.any,

/**
* data, data object from the row
*/
data: PropTypes.object,

/**
* old value of the cell
*/
oldValue: PropTypes.any,

/**
* new value of the cell
*/
newValue: PropTypes.any,

/**
* column where the cell was changed
*/
colId: PropTypes.any,

/**
* Timestamp of when the event was fired
*/
timestamp: PropTypes.any,
})
),

/**
* Other ag-grid options
Expand Down
51 changes: 41 additions & 10 deletions src/lib/fragments/AgGrid.react.js
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,9 @@ const RESIZE_DEBOUNCE_MS = 200;
// Rate-limit for updating columnState when interacting with the grid
const COL_RESIZE_DEBOUNCE_MS = 500;

// Time between syncing cell value changes with Dash
const CELL_VALUE_CHANGED_DEBOUNCE_MS = 1;
emilhe marked this conversation as resolved.
Show resolved Hide resolved

const xssMessage = (context) => {
console.error(
context,
Expand Down Expand Up @@ -110,6 +113,10 @@ function stringifyId(id) {
return '{' + parts.join(',') + '}';
}

function is_undefined_or_null(obj) {
return typeof obj === 'undefined' || obj === null;
}

export default class DashAgGrid extends Component {
constructor(props) {
super(props);
Expand All @@ -119,6 +126,7 @@ export default class DashAgGrid extends Component {
this.onCellClicked = this.onCellClicked.bind(this);
this.onCellDoubleClicked = this.onCellDoubleClicked.bind(this);
this.onCellValueChanged = this.onCellValueChanged.bind(this);
this.afterCellValueChanged = this.afterCellValueChanged.bind(this);
this.onRowDataUpdated = this.onRowDataUpdated.bind(this);
this.onFilterChanged = this.onFilterChanged.bind(this);
this.onSortChanged = this.onSortChanged.bind(this);
Expand Down Expand Up @@ -181,6 +189,7 @@ export default class DashAgGrid extends Component {

this.selectionEventFired = false;
this.reference = React.createRef();
this.pendingChanges = null;
}

onPaginationChanged() {
Expand Down Expand Up @@ -912,20 +921,38 @@ export default class DashAgGrid extends Component {
node,
}) {
const timestamp = Date.now();
// Collect new change.
const newChange = {
rowIndex,
rowId: node.id,
data,
oldValue,
value,
colId,
timestamp,
};
// Append it to current change session.
if (is_undefined_or_null(this.pendingCellValueChanges)) {
emilhe marked this conversation as resolved.
Show resolved Hide resolved
this.pendingCellValueChanges = [newChange];
} else {
this.pendingCellValueChanges.push(newChange);
}
}

afterCellValueChanged() {
// Guard against multiple invocations of the same change session.
if (is_undefined_or_null(this.pendingCellValueChanges)) {
return;
}
// Send update(s) for current change session to Dash.
const virtualRowData = this.virtualRowData();
this.props.setProps({
cellValueChanged: {
rowIndex,
rowId: node.id,
data,
oldValue,
value,
colId,
timestamp,
},
cellValueChanged: this.pendingCellValueChanges,
virtualRowData,
});
this.syncRowData();
// Mark current change session as ended.
this.pendingCellValueChanges = null;
}

onDisplayedColumnsChanged() {
Expand Down Expand Up @@ -1325,7 +1352,11 @@ export default class DashAgGrid extends Component {
onSelectionChanged={this.onSelectionChanged}
onCellClicked={this.onCellClicked}
onCellDoubleClicked={this.onCellDoubleClicked}
onCellValueChanged={this.onCellValueChanged}
onCellValueChanged={debounce(
this.afterCellValueChanged,
CELL_VALUE_CHANGED_DEBOUNCE_MS,
this.onCellValueChanged
)}
onFilterChanged={this.onFilterChanged}
onSortChanged={this.onSortChanged}
onRowDragEnd={this.onSortChanged}
Expand Down
5 changes: 4 additions & 1 deletion src/lib/utils/debounce.js
Original file line number Diff line number Diff line change
@@ -1,11 +1,14 @@
export default function debounce(fn, wait) {
export default function debounce(fn, wait, onDebounce) {
let lastTimestamp = 0;
let handle;

return (...args) => {
const now = Date.now();
const delay = Math.min(now - lastTimestamp, wait);

if (onDebounce) {
onDebounce(...args);
}
if (handle) {
clearTimeout(handle);
}
Expand Down
62 changes: 55 additions & 7 deletions tests/test_cell_value_changed.py
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ def test_cv001_cell_value_changed(dash_duo):
dag.AgGrid(
id="history",
columnDefs=[{"field": "Key", "checkboxSelection": True}]
+ [{"field": i} for i in ["Column", "OldValue", "NewValue"]],
+ [{"field": i} for i in ["Column", "OldValue", "NewValue"]],
rowData=[],
),
],
Expand All @@ -43,12 +43,16 @@ def test_cv001_cell_value_changed(dash_duo):
)

app.clientside_callback(
"""function addToHistory(data) {
if (data) {
reloadData = {...data.data}
reloadData[data.colId] = data.oldValue
newData = [{Key: data.rowId, Column: data.colId, OldValue: data.oldValue, NewValue: data.value,
reloadData}]
"""function addToHistory(changes) {
if (changes) {
newData = []
for (let i = 0; i < changes.length; i++) {
data = changes[i];
reloadData = {...data.data};
reloadData[data.colId] = data.oldValue;
newData.push({Key: data.rowId, Column: data.colId, OldValue: data.oldValue,
NewValue: data.value, reloadData});
}
return {'add': newData}
}
return window.dash_clientside.no_update
Expand Down Expand Up @@ -100,3 +104,47 @@ def test_cv001_cell_value_changed(dash_duo):
grid.get_cell(1, 2).click()

hist.wait_for_rendered_rows(1)


def test_cv001_cell_value_changed_multi(dash_duo):
app = Dash(__name__)
app.layout = html.Div(
[
dag.AgGrid(
columnDefs=columnDefs,
rowData=df.to_dict("records"),
columnSize="sizeToFit",
defaultColDef={"editable": True},
id="grid",
getRowId="params.data.nation",
dashGridOptions={'editType':'fullRow'}
),
html.Div(id="log")
]
)

app.clientside_callback(
"""function countEvents(changes) {
console.log("FIRE");
return changes? changes.length : 0;
}""",
Output("log", "children"),
Input("grid", "cellValueChanged"),
prevent_initial_call=True,
)

dash_duo.start_server(app)

grid = utils.Grid(dash_duo, "grid")
grid.wait_for_cell_text(0, 0, "South Korea")

# Test single event.
grid.get_cell(0, 1).send_keys("50")
grid.get_cell(1, 2).click()
dash_duo.wait_for_text_to_equal('#log', "1")

# Test multi event.
grid.get_cell(0, 1).send_keys("20")
grid.get_cell_editing_input(0, 2).send_keys("20")
grid.get_cell(1, 2).click()
dash_duo.wait_for_text_to_equal('#log', "2")
5 changes: 5 additions & 0 deletions tests/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -170,6 +170,11 @@ def get_cell(self, row, col):
f'#{self.id} .ag-row[row-index="{row}"] .ag-cell[aria-colindex="{col + 1}"]'
)

def get_cell_editing_input(self, row, col):
return self.dash_duo.find_element(
f'#{self.id} .ag-row[row-index="{row}"] .ag-cell[aria-colindex="{col + 1}"] .ag-cell-editor input'
)

def get_row(self, row):
return self.dash_duo.find_element(f'#{self.id} .ag-row[row-index="{row}"]')

Expand Down