POC: Generic way of making Grid rows editable

[jamesricky]

TLDR

Allow editing rows of a DataGrid by clicking on the row directly, in addition to the edit button.
Add tests to make sure clicking in interactive cells does not trigger the edit-action.

This adds a new hook useEditGridRow

The hook returns the following:

• handleDataGridRowClick: the function to be passed to onRowClick of the DataGrid to allow editing the row by clicking anywhere in the row.
• EditIconButton: The IconButton that should be rendered in the actions cell, which is used to edit the row. This should be used when other, additional actions should be rendered inside the actions cell.
• actionsCell the full actions-cell that includes the EditIconButton, this should be used when the actions-cell should include only the edit button.

onRowClick and EditIconButton intentionally do not share a function

The IconButton uses StackLink to render an actual link (<a href="..." />), which is preferred due to it's default accessibility behavior, e.g. using cmd+click/ctrl+click or the system context-menu to open the link in a new tab.

DataGrid does not support an actual link for it's row-click, therefore we must use a function here.
This function manually adds support for cmd+click/ctrl+click to open the link in a new tab, as this is likely a common use-case. Other accessibility-features of link are not supported when clicking the row.

Interactive elements within a cell

If the onRowClick prop is set, clicking anywhere within a cell will trigger this function.

• This correctly includes static elements like text-content.
• This incorrectly includes interactive elements such as buttons, selects, inputs.

To avoid this, we manually set type: "actions" on every interactive cell, the same as we already do in the cell that includes "edit" and other icon-buttons.

Negatives of using type: "actions"

• Needs to be set on every interactive column manually
• Could break existing, generated grids: Custom, interactive columns won't include type: "actions", will therefore trigger the function instead of allowing the interaction
  • We could set type: "actions" on all custom columns by default, as they will likely often be interactive anyway.
  • We could omit onRowClick on generated grids, if they include custom columns (at least until V9)
• Columns with type: "actions" will not be sortable or filterable by default
  • Columns would additionally require sortable: true and filterable: true
  • This might be the desired behavior for many interactive columns
  • It may be an issue that you can't do things by default such as sort by visible or filter by description
• Would require adjusting the text-alignment of the cell and header-cell

Alternative to type: "actions"

We could wrap the content of each renderCell with an element that does event.stopPropagation() when clicked.
We could also possibly abstract this away by adding an interactive: true flag to GridColDef.

• Requires using the wrapper-component or setting interactive: true on every interactive column
• Will not solve the above mentioned issue about breaking generated grids
• Will allow filtering/sorting by default
• Causes no text-alignment issue in the cell and header-cell
• Triggers onRowClick when clicking within the cell but not on the interactive part, e.g. 1px next to the visible select

const InteractiveCellWrapper = ({ children }: { children: React.ReactNode }) => {
    return (
        <Box sx={{ display: "contents" }} onClick={(event) => event.stopPropagation()}>
            {children}
        </Box>
    );
};

const columns: GridColDef[] = [
    {
        field: "interactiveValue",
        renderCell: () => (
            <InteractiveCellWrapper>
                <SomeInteractiveElement />
            </InteractiveCellWrapper>
        ),
    },
    // ...

Screenshots/screencasts

Simple.grid.mov

Grid.with.interactive.cells.mov

Open TODOs

• Merge parent and rebase Set cursor of DataGrid rows to "pointer" if onRowClick is set #4933
• If we decide to go with this method: Create tasks for the follow-up tasks

Questions

• Do we want useEditGridRow in @comet/admin?
• Decide on type: "actions" vs. InteractiveCellWrapper vs. interactive: true flag

Follow-up tasks

• Export useEditGridRow from @comet/admin and use it in these stories
• Document the usage, mentioning the necessary settings for interactive cells
• Use useEditGridRow in Admin-Generator in a way that won't break custom interactive cells (maybe set type: "actions" by default on all custom cells)

If we go with type: "actions":

• Fix the text-alignment of the cell and header-cell when using type: "actions"

If we go with InteractiveCellWrapper or interactive: true:

• Implement this in @comet/admin

Further information

• Task: https://vivid-planet.atlassian.net/browse/COM-2618

[johnnyomair]

Do we need this new hook? It seems to me that the only "new" code is opening the detail page in the on click handler. The rest looks like the code we already generate? What's you motivation for this hook?

[jamesricky]

Do we need this new hook?

Not necessarily but it seems to me like a hook is easier than writing (copy-pasting) all that code into every grid.

It seems to me that the only "new" code is opening the detail page in the on click handler.

True, the rest (except for the aria-label) can be generated by the admin-generator.

What's you motivation for this hook?

• Less code for non-generated grids: All three returned values would have to be done manually
• A consistent Edit-Button: devs can't forget the aria-label or the correct color or render a less accessible <button /> instead of the StackLink (<a />)

[johnnyomair]

This is something the Admin Generator could generate.

[jamesricky]

We also need a solution for non-generated grids. I think whenever implementing this manually, devs won't always think of adding the shouldOpenLinkInNewTab logic.

[johnnyomair]

We could set type: "actions" on all custom columns by default, as they will likely often be interactive anyway.

I'm not sure if this is a good idea since it breaks other features like sorting and filtering.

[johnnyomair]

Isn't this issue (two actions with overlapping tap targets) solvable using different z-indices? 🤔

[jamesricky]

Isn't this issue (two actions with overlapping tap targets) solvable using different z-indices? 🤔

No, when multiple nested elements have an onClick, clicking the inner element will always trigger onClick on the parents, even if positioned away/above the outer element.

[jamesricky]

We could set type: "actions" on all custom columns by default, as they will likely often be interactive anyway.

I'm not sure if this is a good idea since it breaks other features like sorting and filtering.

This shouldn't be an issue if we go with the interactive: true alternative instead.
We could also make the whole feature opt-in on generated grids, at least in V8.

[johnnyomair]

The data grid's primary action may not always be edit. For instance, in the cron jobs module, the primary action is to start a cron job (now that I think of it, it's probably no good idea to start the cron job on row click 😅). Maybe we should go with a useDataGridPrimaryAction approach instead?

We should discuss this in the next Comet JFX.

[johnnyomair]

This shouldn't be an issue if we go with the interactive: true alternative instead.

I'm not sure if further extending/changing the grid column definition is a good idea. This moves Comet further away from MUI, making it IMO more confusing.