Repository navigation
[JSX] Add RecordTable and move candidate_list onto it - #11277
HenriRabalais wants to merge 15 commits into
Conversation
…tokens # Conflicts: # jsx/Form.js
| export type Column<R> = { | ||
| name: string, | ||
| label: string, | ||
| value: (record: R) => any, | ||
| display?: (value: any, record: R) => string, | ||
| format?: (value: any, record: R) => ReactNode, | ||
| type?: ColumnType, | ||
| filter?: ColumnFilter, | ||
| hidden?: boolean, | ||
| groupable?: boolean, | ||
| sticky?: boolean, | ||
| group?: string, | ||
| }; |
There was a problem hiding this comment.
I am a little annoyed by the any type here, as it means type checking will not be able to check if values are used correctly within each column, and is exactly one of the bugs you encountered in one of your recent bug fix PRs.
I made this small experiment that shows how we can actually have type checking for value types within each column. The main issues are that the implementation is a little complex (we must encode existential types using a continuation passing style since Typescript does not have them as a first-class feature) and that the user must call col(/* column definition */) on each column, but it works without any cast, without any, and any mistake in a column definition or manipulation would result in a type error. Tell me if you have any thought.
There was a problem hiding this comment.
thanks for putting the playground together, it makes the problem really clear.
I like the idea of checking each column against its own value type. I tried a lighter version of it in the playground: if Column<R, V> declares value and format as methods, a small col<R, V>(column): Column<R, unknown> helper that just returns its argument is enough. Each column is checked on its own (a format that doesn't match its value is a type error, and V is inferred), and the columns still sit in one array without a cast or any continuation passing. Do you see something that approach misses?
I'm leaning towards doing that, which would turn the any in RecordTable.tsx, filterMatch.ts and tableState.ts into unknown. The modules are plain JS today, so it would only help TypeScript callers for now.
There was a problem hiding this comment.
I like the idea of checking each column against its own value type. I tried a lighter version of it in the playground: if Column<R, V> declares value and format as methods, a small col<R, V>(column): Column<R, unknown> helper that just returns its argument is enough. Each column is checked on its own (a format that doesn't match its value is a type error, and V is inferred), and the columns still sit in one array without a cast or any continuation passing. Do you see something that approach misses?
Hard to say without seeing an implementation, but from the description that would solve the issue. 👍
Description
DataTable takes each row as an array matched to
fieldsby position, and a module formats cells throughgetFormattedCell, which only sees that row's values. As suggested on #11251, this adds a new component rather than changing DataTable.RecordTabletakes the records themselves, and each column is one object withvalue,displayandformat, so a column is defined in one place and the filter, the CSV and the screen all read from it.It also reworks the toolbar and footer from #11249. Filters sit in a drawer on the left, a clicked record opens in a panel on the right, and one toolbar holds the table's name, its saved views, search, layout, grouping, columns, the CSV download and the table's own actions. Create and edit forms open in the same right-hand panel.
candidate_list is the first module on it. new_profile's form is split from its page so the candidate list can open it from the table. The other modules stay on DataTable and move over one PR each.
Changes
RecordTableinjsx/table/, with columns defined byvalue,displayandformat.jsx/table/filterMatch.tsso both tables share it.RecordTableand updated its integration test.NewProfileForm.jsso it can open from the candidate list.Testing Instructions
/candidate_list/?project=Pumpernickelstill filters.Related Issues
Part of #11249. Supersedes #11252.