feat: edit data in nested table (PT-188211532) #40
Conversation
emcelroy
force-pushed
the
188211532-edit-data-in-nested-table
branch
from
e5b9abf
to
e8b6937
Compare
There was a problem hiding this comment.
This looks good, but I do think it would be worth updating some types before merging.
I know you didn't add the anys that I pointed out, but this would be a good opportunity to fix them. I mentioned that you could just do something like type Case = any, but I bet something like this would work:
interface Case {
id: number
values: Values
}
I also think it would be better to use Cases instead of CaseValuesWithId.
I'm approving because I don't think this should hold up the PR if there's any urgency, but I do think it would be worth spending an hour or two to make these changes.
|
|
||
| export const EditableTableCell = (props: IProps) => { | ||
| const { attrTitle, caseId, children, handleUpdateCollections, selectedDataSetName } = props; | ||
| const displayValue = String(children); |
There was a problem hiding this comment.
Would it make sense to make this a displayValue: string prop, rather than casting a special ReactNode?
src/components/flat-table.tsx
Outdated
| import { DraggableTableContainer, DraggagleTableHeader } from "./draggable-table-tags"; | ||
| import { getAttrPrecisions, getAttrTypes, getAttrVisibility } from "../utils/utils"; | ||
|
|
||
| import css from "./tables.scss"; | ||
|
|
||
| interface IFlatProps extends ITableProps { | ||
| items: Array<any> | ||
| cases: Array<any> |
There was a problem hiding this comment.
It would be great to start removing any types. Even if you just define type Case = any in types.ts, this would at least allow future engineers to see where cases appear in the codebase. But maybe you can do better than that?
src/components/nested-table.tsx
Outdated
| @@ -18,21 +18,22 @@ interface IProps { | |||
| selectedDataSet: any; | |||
| dataSets: IDataSet[]; | |||
| collections: ICollections; | |||
| items: any[]; | |||
| cases: any[]; | |||
| return Object.keys(aCase).map((key, index) => { | ||
| if (key === "id") return null; |
There was a problem hiding this comment.
I get the feeling that it would be simpler if you passed a Case object in, instead of a Values object with a special id value. Then you could just map through aCase.values.
|
@tealefristoe Thanks 👍 I started improving the typing a bit. I'm pretty sure your other suggestions make sense, but I want to consult with @lublagg when she's back next week before implementing them. Since we're going to continue working in the same parts of the code, I'm going to merge this and we'll possibly make the other changes in the next PR. |
#188211532