Skip to content

Commit b743b94

Browse files
schloerkeschloerke
committed
Followup from Joe's comments
* Fix conflicting click issue when clicking a cell from an editable cell * Rename `on_cell{s}_update` -> `set_cell{s}_update_fn` * Remove `kwargs` from `set_cell{s}_update_fn()` * outputRPC handler gives better error message. If sanitization is turned on, a generic message is returned on error. * Rename `formatted_values` var to `updated_raw_values`
1 parent 5c82fe2 commit b743b94

File tree

8 files changed

+135
-121
lines changed

8 files changed

+135
-121
lines changed

‎js/dataframe/cell.tsx‎

Lines changed: 77 additions & 54 deletions
Original file line numberDiff line numberDiff line change
@@ -111,7 +111,14 @@ export const TableBodyCell: FC<TableBodyCellProps> = ({
111111

112112
const [cellState, setCellState] = useState<CellState>(CellState.Ready);
113113

114-
const editable = editRowIndex === rowIndex && editColumnIndex === columnIndex;
114+
const [editable, setEditable] = useState(false);
115+
useEffect(() => {
116+
const cellIsEditable =
117+
editRowIndex === rowIndex && editColumnIndex === columnIndex;
118+
setEditable(cellIsEditable);
119+
}, [editRowIndex, editColumnIndex, rowIndex, columnIndex]);
120+
121+
// const editable = editRowIndex === rowIndex && editColumnIndex === columnIndex;
115122
if (editable) {
116123
setCellState(CellState.Editing);
117124
}
@@ -189,17 +196,24 @@ export const TableBodyCell: FC<TableBodyCellProps> = ({
189196

190197
const tableCellClass = tableCellMap[cellState];
191198

199+
const [errorTitle, setErrorTitle] = useState<string | null>(undefined);
200+
192201
// useEffect(() => {
193202
// console.log("Cell background:", tableCellClass, rowIndex, columnIndex);
194203
// }, [tableCellClass, rowIndex, columnIndex]);
195204
// useEffect(() => {
196205
// console.log("Cell state:", cellState, hasUpdated);
197206
// }, [hasUpdated, cellState]);
198207

199-
function resetEditInfo() {
208+
// function resetEditInfo() {
209+
// console.log("Resetting edit info!", rowIndex, columnIndex);
210+
// setEditRowIndex(null);
211+
// setEditColumnIndex(null);
212+
// }
213+
const resetEditInfo = React.useCallback(() => {
200214
setEditRowIndex(null);
201215
setEditColumnIndex(null);
202-
}
216+
}, [editRowIndex, editColumnIndex]);
203217

204218
const onEsc = (e: ReactKeyboardEvent<HTMLInputElement>) => {
205219
if (e.key !== "Escape") return;
@@ -232,8 +246,6 @@ export const TableBodyCell: FC<TableBodyCellProps> = ({
232246

233247
const hasShift = e.shiftKey;
234248

235-
console.log(maxRowSize);
236-
237249
const newRowIndex = editRowIndex + (hasShift ? -1 : 1);
238250
if (newRowIndex < 0 || newRowIndex >= maxRowSize) {
239251
// If the new row index is out of bounds, quit
@@ -250,34 +262,34 @@ export const TableBodyCell: FC<TableBodyCellProps> = ({
250262

251263
const attemptUpdate = () => {
252264
// Only update if the string form of the value has changed
253-
if (`${initialValue}` !== `${value}`) {
254-
setCellState(CellState.EditSaving);
255-
// console.log("Setting update count");
256-
// setUpdateCount(updateCount + 1);
257-
updateCellsData({
258-
id,
259-
cellInfos: [
260-
{
261-
rowIndex,
262-
columnIndex,
263-
value,
264-
prev: initialValue,
265-
},
266-
],
267-
onSuccess: (values) => {
268-
// console.log("success!", values);
269-
setCellState(CellState.EditSuccess);
270-
},
271-
onError: (err) => {
272-
// console.log("error!", err);
273-
console.error("Error saving tabel cell value!", err);
274-
setCellState(CellState.EditFailure);
265+
if (`${initialValue}` === `${value}`) return;
266+
267+
setCellState(CellState.EditSaving);
268+
// console.log("Setting update count");
269+
// setUpdateCount(updateCount + 1);
270+
updateCellsData({
271+
id,
272+
cellInfos: [
273+
{
274+
rowIndex,
275+
columnIndex,
276+
value,
277+
prev: initialValue,
275278
},
276-
columns,
277-
setData,
278-
setCellEditMap,
279-
});
280-
}
279+
],
280+
onSuccess: (values) => {
281+
// console.log("success!", values);
282+
setCellState(CellState.EditSuccess);
283+
},
284+
onError: (err) => {
285+
console.error("Error saving tabel cell value!", err);
286+
setErrorTitle(String(err));
287+
setCellState(CellState.EditFailure);
288+
},
289+
columns,
290+
setData,
291+
setCellEditMap,
292+
});
281293
};
282294

283295
// When the input is blurred, we'll call our table meta's updateData function
@@ -294,27 +306,28 @@ export const TableBodyCell: FC<TableBodyCellProps> = ({
294306

295307
// Select the input when it becomes editable
296308
useEffect(() => {
297-
if (editable) {
298-
inputRef.current.focus();
299-
inputRef.current.select();
309+
if (!editable) return;
300310

301-
// Setup global click listener to reset edit info
311+
inputRef.current.focus();
312+
inputRef.current.select();
313+
314+
// Setup global click listener to reset edit info
315+
const onBodyClick = (e: MouseEvent) => {
316+
if (e.target === inputRef.current) {
317+
return;
318+
}
319+
resetEditInfo();
320+
};
321+
window.document
322+
.querySelector("body")
323+
.addEventListener("click", onBodyClick);
324+
325+
// Tear down global click listener when we're done
326+
return () => {
302327
window.document
303328
.querySelector("body")
304-
.addEventListener("click", (e: MouseEvent) => {
305-
// console.log("body click!", e.target, inputRef.current);
306-
// TODO-barret; Do not reset if target is another table cell!
307-
// Or skip the reset?
308-
resetEditInfo();
309-
});
310-
311-
// Tear down global click listener when we're done
312-
return () => {
313-
window.document
314-
.querySelector("body")
315-
.removeEventListener("click", resetEditInfo);
316-
};
317-
}
329+
.removeEventListener("click", onBodyClick);
330+
};
318331
}, [editable]);
319332

320333
// Reselect the input when it comes into view!
@@ -326,18 +339,20 @@ export const TableBodyCell: FC<TableBodyCellProps> = ({
326339
}
327340

328341
function onChange(e: ReactChangeEvent<HTMLInputElement>) {
329-
console.log("on change!");
342+
// console.log("on change!");
330343
setValue(e.target.value);
331344
}
332345

333346
let onClick = undefined;
334347
let content = undefined;
348+
let cellTitle = errorTitle;
335349

336350
if (cellState === CellState.EditSaving) {
337351
content = <em>{value as string}</em>;
338352
} else if (editable) {
339353
content = (
340354
<input
355+
className="cell-edit-input"
341356
value={value as string}
342357
onChange={onChange}
343358
onBlur={onBlur}
@@ -348,18 +363,26 @@ export const TableBodyCell: FC<TableBodyCellProps> = ({
348363
);
349364
} else {
350365
onClick = (e: ReactMouseEvent<HTMLTableCellElement>) => {
351-
console.log("on ready click!", e.target);
352366
setEditRowIndex(rowIndex);
353367
setEditColumnIndex(columnIndex);
368+
e.preventDefault();
369+
e.stopPropagation();
354370
};
355371
if (cellState === CellState.EditFailure) {
356-
console.log("Render edit failure");
372+
// TODO-barret; Handle edit failure?
373+
// console.log("Render edit failure");
374+
cellTitle = errorTitle;
357375
}
358376
content = flexRender(cell.column.columnDef.cell, cell.getContext());
359377
}
360378

361379
return (
362-
<td key={cell.id} onClick={onClick} className={tableCellClass}>
380+
<td
381+
key={cell.id}
382+
onClick={onClick}
383+
title={cellTitle}
384+
className={tableCellClass}
385+
>
363386
{content}
364387
</td>
365388
);

‎js/dataframe/data-update.tsx‎

Lines changed: 0 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -60,14 +60,6 @@ export function updateCellsData({
6060
setData((draft) => {
6161
values.forEach((value: string, i: number) => {
6262
const { rowIndex, columnIndex } = cellInfos[i];
63-
// const row = draft[rowIndex];
64-
// // console.log(
65-
// // "Setting new value!",
66-
// // value,
67-
// // columnId,
68-
// // draft[rowIndex]
69-
// // );
70-
7163
draft[rowIndex][columnIndex] = value;
7264
});
7365
});

‎js/dataframe/index.tsx‎

Lines changed: 3 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -119,9 +119,9 @@ const ShinyDataGrid: FC<ShinyDataGridProps<unknown>> = (props) => {
119119
const [editRowIndex, setEditRowIndex] = useState<number>(null);
120120
const [editColumnIndex, setEditColumnIndex] = useState<number>(null);
121121

122-
useEffect(() => {
123-
console.log("editing info!", editRowIndex, editColumnIndex);
124-
}, [editColumnIndex, editRowIndex]);
122+
// useEffect(() => {
123+
// console.log("editing info!", editRowIndex, editColumnIndex);
124+
// }, [editColumnIndex, editRowIndex]);
125125

126126
const dataFrameModeIsMissing = data.options["mode"] ? false : true;
127127
const dataFrameMode = data.options["mode"] ?? "none";
@@ -133,10 +133,6 @@ const ShinyDataGrid: FC<ShinyDataGridProps<unknown>> = (props) => {
133133
>(new Map<string, { value: string; state: CellState }>());
134134
enableMapSet();
135135

136-
useEffect(() => {
137-
console.log("editing info!", editRowIndex, editColumnIndex);
138-
}, [editColumnIndex, editRowIndex]);
139-
140136
const coldefs = useMemo<ColumnDef<unknown[], unknown>[]>(
141137
() =>
142138
columns.map((colname, i) => {

‎shiny/render/_dataframe.py‎

Lines changed: 14 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -40,8 +40,6 @@ async def __call__(
4040
self,
4141
*,
4242
info: CellUpdateInfo,
43-
# TODO-barret; Remove `**kwargs`
44-
**kwargs: Any, # future proofing
4543
) -> Any: ...
4644

4745

@@ -57,14 +55,10 @@ async def __call__(
5755
self,
5856
*,
5957
infos: list[CellUpdateInfo],
60-
# TODO-barret; Remove `**kwargs`
61-
**kwargs: Any, # future proofing
6258
) -> Any: ...
6359
@dataclass
6460
class CellPatch:
6561
row_index: int
66-
# TODO-barret; Safeguard against columns with the same name
67-
# TODO-barret; Use column index instead of name
6862
column_index: int
6963
value: str
7064
prev: str
@@ -537,17 +531,16 @@ def _get_session(self) -> Session:
537531
)
538532
return self._session
539533

540-
# TODO-barret; `on` sounds like a registration. Maybe use `set` instead? `set_cell_update_fn`?
541-
def on_cell_update(self, fn: CellUpdateFn) -> Self:
534+
def set_cell_update_fn(self, fn: CellUpdateFn) -> Self:
542535
self.handle_cell_update = fn
543536
return self
544537

545-
def on_cells_update(self, fn: CellsUpdateFn) -> Self:
538+
def set_cells_update_fn(self, fn: CellsUpdateFn) -> Self:
546539
self.handle_cells_update = fn
547540
return self
548541

549542
def _init_handlers(self) -> None:
550-
async def _on_cell_update_default(
543+
async def _set_cell_update_default(
551544
*,
552545
info: CellUpdateInfo,
553546
# row_index: int,
@@ -558,7 +551,7 @@ async def _on_cell_update_default(
558551
) -> str:
559552
return info["value"]
560553

561-
async def _on_cells_update_default(
554+
async def _set_cells_update_default(
562555
*,
563556
infos: list[CellUpdateInfo],
564557
**kwargs: Any,
@@ -583,12 +576,13 @@ async def _on_cells_update_default(
583576

584577
return formatted_values
585578

586-
self.on_cell_update(_on_cell_update_default)
587-
self.on_cells_update(_on_cells_update_default)
579+
self.set_cell_update_fn(_set_cell_update_default)
580+
self.set_cells_update_fn(_set_cells_update_default)
588581
# self._add_message_handlers()
589582

590583
# To be called by session's outputRPC message handler on this data_frame
591584
# Do not change this method unless you update corresponding code in `/js/dataframe/`!!
585+
# TODO-barret; mark functions for outputRPC
592586
# @outputRPC_handler
593587
async def _handle_cells_update(self, update_infos: list[CellUpdateInfo]):
594588

@@ -598,22 +592,21 @@ async def _handle_cells_update(self, update_infos: list[CellUpdateInfo]):
598592
# Make new array to trigger reactive update
599593
patches = [p for p in self.cell_patches()]
600594

601-
# Call user's on_cells_update method to retrieve formatted values
602-
# TODO-barret; REname `formatted` to `raw_values`
603-
formatted_values = await self.handle_cells_update(infos=update_infos)
595+
# Call user's cell update method to retrieve formatted values
596+
updated_raw_values = await self.handle_cells_update(infos=update_infos)
604597

605-
if len(formatted_values) != len(update_infos):
598+
if len(updated_raw_values) != len(update_infos):
606599
raise ValueError(
607-
f"The return value of {self.output_id}'s `handle_cells_update()` (typically set by `@{self.output_id}.on_cells_update`) must be a list of the same length as the input list of cell updates. Received {len(formatted_values)} items and expected {len(update_infos)}."
600+
f"The return value of {self.output_id}'s `handle_cells_update()` (typically set by `@{self.output_id}.set_cells_update_fn`) must be a list of the same length as the input list of cell updates. Received {len(updated_raw_values)} items and expected {len(update_infos)}."
608601
)
609602

610603
# Add new patches
611-
for formatted_value, update_info in zip(formatted_values, update_infos):
604+
for updated_raw_value, update_info in zip(updated_raw_values, update_infos):
612605
patches.append(
613606
CellPatch(
614607
row_index=update_info["row_index"],
615608
column_index=update_info["column_index"],
616-
value=formatted_value,
609+
value=updated_raw_value,
617610
prev=update_info["prev"],
618611
)
619612
)
@@ -627,7 +620,7 @@ async def _handle_cells_update(self, update_infos: list[CellUpdateInfo]):
627620
# Set new patches
628621
self.cell_patches.set(patches)
629622

630-
return formatted_values
623+
return updated_raw_values
631624

632625
def auto_output_ui(self) -> Tag:
633626
return ui.output_data_frame(id=self.output_id)
@@ -683,8 +676,6 @@ async def render(self) -> Jsonifiable:
683676
return value.to_payload()
684677

685678
async def _send_message_to_browser(self, handler: str, obj: dict[str, Any]):
686-
# TODO-barret; capture data in render method
687-
# TODO-barret; invalidate data in render method before user fn has been called
688679

689680
session = self._get_session()
690681
id = session.ns(self.output_id)

0 commit comments

Comments
 (0)