Edit to Table Component

[mayakoneval]

• no console errors about keys
• allow header titles to be react components
• no bottom border on last row
• default to full width

[cheapsteak]

I don't think we should check in the compiled results of Table.d.ts, Table.js etc
I see that we have Button.js/d.ts/js.map but I think those were almost certainly accidentally committed and we should remove them
That's the only blocking issue with the PR

If the main motivation for this is PR is to remove the border on the bottom row, then let's not block it on the other key related issues since one is visible to users and the others are future concerns (albeit near-future)

[cheapsteak]

I don't think we wanted to check in compiled code did we?

If we want to exclude the compiled code (I can't think of a reason we would want to check this in), we can reference the .gitignore for what Justin did for colors and typography

[cheapsteak]

Ah good idea adding keys, we do need this

We shouldn't use the positional index as the key though (this is actually the same as not specifying the prop at all)

Say we have 3 rows (A, B, C), and we were to delete A to result in 2 rows of (B, C), we'll end up rendering (A, B) because according to the keys, 3 was removed, 1 and 2 haven't changed. (a longer article on this)

One solution would be to have <Table/> accept a keyOn prop (analogous to react-table's indexKey prop), then we can <tr key={item[props.keyOn]}/>

[mayakoneval]

@cheapsteak typescript wont allow us to access item[props.keyOn] unless keyOn is a single type of RowShape. I don't know how to type something as "one of the fields of a type", I know Partial exists but you can't access an element on a Partial typed key. Thoughts?

[cheapsteak]

How's this?

@@ -36,14 +36,18 @@ interface Props<RowShape> {
       list: ReadonlyArray<RowShape>
     ) => React.ReactNode;
   }>;
+
+  keyOn: keyof RowShape | ((row: RowShape) => any);
 }
 
 export function Table<RowShape>({
   data,
   density = "standard",
   columns,
+  keyOn,
 }: Props<RowShape>): ReturnType<React.FC> {
   const padding = density === "standard" ? 8 : density === "condensed" ? 3 : 11;
+  const getRowKey = typeof keyOn === 'function' ? keyOn : (row) => row[keyOn];
 
   return (
     <table
@@ -77,7 +81,7 @@ export function Table<RowShape>({
       </thead>
       <tbody>
         {data.map((item, index) => (
-          <tr key={index}>
+          <tr key={getRowKey(item)}>
             {columns.map(
               ({
                 render,

(dont see any ts errors in my editor at least)

update: edited

[mayakoneval]

oo nice! thanks

[justinanastos]

My advice would be to go even more extreme: provide a function to get the key and if that function doesn't exist, then use JSON.stringify on the whole row.

[cheapsteak]

Same issue here, the key shouldn't be the positional index (this index also isn't the rowIndex but is rather the columnIndex)

We should require that each column specify its own id (in addition to headerTitle and render (the ambiguity of this render prop might be problematic if we want to differentiate between render functions for cells and headers nvm I see that's what "headerTitle" is))

[cheapsteak]

thanks for deleting Button compiled files too!

Out of scope for this PR but we should think about how we might be able to gitignore these without having to manually specify each file (bothersome). Maybe we could ignore all .js, .d.ts, and .map files?

[justinanastos]

I was being lazy not adding any CI to this project. If we're going to start having whitespace and quote style changes, it's time for CI and a prettier config.