Conversation
| return ( | ||
| <TableRow | ||
| key={row.id} | ||
| hover |
There was a problem hiding this comment.
Should we enable hover by default? It can be useful as a visual guide just for reading long rows without getting lost.
There was a problem hiding this comment.
The hover interaction might signal to the end user that it's intractable.
There was a problem hiding this comment.
Also the cursor change in 4f2c9a0 / #1360 (comment) is tied to hover.
There was a problem hiding this comment.
hover background change alone does not signal to the user that it's interactable. We used a hover background on all rows in the legacy Table just as a visual guide. "Interactable" row does not even make sense on its own; there should always be an explicit action (e.g. a checkbox in this case, or a button as shown in iTwinUI docs).
Ideally we would use the hover background with the default cursor, and add cursor: pointer when the row is selectable. Is this possible?
There was a problem hiding this comment.
I can look for .MuiTableCell-paddingCheckbox within the row and apply the cursor. I think if the checkbox column is present then that must mean the row is selectable. ce355d9
There was a problem hiding this comment.
And what about hover as default?
There was a problem hiding this comment.
I tried adding hover to TableRow but that gave the header row hover as well:
MuiTableRow: {
defaultProps: { component: withRenderProp(Role, "tr"), hover: true },
},Basically I need to check "if the TableRow is within TableBody then change the hover default to true". So now I'm exploring what I can do with MuiTableBody.
There was a problem hiding this comment.
Ah true. I can open a follow-up PR so we can keep this moving.
Changes
Deploy preview 👀
Screenshots show:
Testing
forced-colorsis matching legacy StrataKit table. Would of liked to style the selected row differently, but the checkbox will suffice.