feat: UI table respond to non-primitive prop changes #1046
Conversation
mattrunyon
requested review from
dgodinez-dh and
mofojed
and removed request for
a team and
dgodinez-dh
|
|
||
| const log = Log.module('@deephaven/js-plugin-ui/UITable'); | ||
|
|
||
| function useDeepEquals<T>(value: T): T { |
There was a problem hiding this comment.
It might be worth placing this in it's own file so it can be used in other code.
There was a problem hiding this comment.
I agree. I think this and the other hook would be good in @deephaven/react-hooks, but I was being lazy and avoiding needing to merge to web-client-ui, publish, then consume here. I can do that though
There was a problem hiding this comment.
Add JS docs for this hook as well.
| !array.every((v, i) => v === stableArray.current[i]) | ||
| ) { | ||
| stableArray.current = array; | ||
| function useThrowError(): [ |
| @@ -22,27 +22,63 @@ import { type dh as DhType } from '@deephaven/jsapi-types'; | |||
| import { ColorGradient, DatabarConfig, FormattingRule } from './UITableUtils'; | |||
| import JsTableProxy, { UITableLayoutHints } from './JsTableProxy'; | |||
|
|
|||
| /** | |||
| * Create a UITableModel. | |||
| * WARNING: This may mutate the baseTable object to add customColumns for formatting. | |||
There was a problem hiding this comment.
Should we copy the baseTable to avoid mutating it? Or is it desirable for other code to have it mutated?
There was a problem hiding this comment.
I think you're right. Initially I was incorrectly thinking if I copied the table the original might have a subscription that gets left open. But if that's the case, it would be up to the caller to handle that subscription anyway.
| }) { | ||
| this.table = table; | ||
| this.originalCustomColumns = table.customColumns; |
There was a problem hiding this comment.
May just want to do a table.copy() and apply custom columns to that/use that table so the original table doesn't get messed with.
|
|
||
| const log = Log.module('@deephaven/js-plugin-ui/UITable'); | ||
|
|
||
| function useDeepEquals<T>(value: T): T { |
There was a problem hiding this comment.
Add JS docs for this hook as well.
|
|
||
| const alwaysFetchColumns = useMemo(() => { | ||
| if (alwaysFetchColumnsArray[0] === true) { | ||
| if (modelColumns.length > 500) { | ||
| if (columns.length > 500) { |
There was a problem hiding this comment.
nit: The 500 should be a constant, as it's used in the error message below.
| setError( | ||
| `Table has ${modelColumns.length} columns, which is too many to always fetch. ` + | ||
| `Table has ${columns.length} columns, which is too many to always fetch. ` + |
There was a problem hiding this comment.
You should use the useThrowError hook above here. Looks like you only used in useUITableModel even though it applies here as well.
Fixes #982
Also fixes a bug I found while implementing this where adding a custom column in the UI would cause formatting rule
if_clauses to be removed.Also noticed I had copy-pasted the "UI table formatting" tests from "UI flex" and forgot to change the describe title, so I corrected the test name (that is the majority of the snapshot changes)