Add "Parents" Columns #4691
Add "Parents" Columns #4691
Conversation
julieg18
force-pushed
the
add-flattened-table-column
branch
from
08bc33d
to
274569f
Compare
| export const COMMIT_COLUMN_ID = 'commit' | ||
| export const BRANCH_COLUMN_ID = 'branch' | ||
|
|
||
| export const DEFAULT_COLUMN_IDS = [ |
There was a problem hiding this comment.
Added two more default columns (ids are commt and branch). These always stay in the order on the backend and appear on the frontend if the table is sorted. Similar to the id column, they are not draggable and can't be unselected.
| @@ -108,7 +112,9 @@ export const ExperimentsTable: React.FC = () => { | |||
| toggleAllRowsExpanded() | |||
| }, [toggleAllRowsExpanded]) | |||
|
|
|||
| const hasOnlyDefaultColumns = columns.length <= 1 | |||
| const hasOnlyDefaultColumns = columns.every( | |||
| ({ id }) => id && isDefaultColumn(id) | |||
There was a problem hiding this comment.
Updated this to check for the column ids since there are now 1 or 3 default columns depending on if the table is sorted.
There was a problem hiding this comment.
[Q] Does every return true if the array is empty?
| @@ -25,7 +26,7 @@ import { vsCodeApi } from '../../shared/api' | |||
| import { | |||
| commonColumnFields, | |||
| expectHeaders, | |||
| tableData as sortingTableDataFixture | |||
| tableData as simplifiedSortedTableDataFixture | |||
There was a problem hiding this comment.
Renamed this to differentiate from the sortedTableDataFixture which has data based on the expShow data. I did consider replacing simplifiedSortedTableData with the backend one, but that would complicate multiple tests so I decided to keep it.
webview/src/experiments/components/table/content/CommitHeader.tsx
Outdated
Show resolved
Hide resolved
| @@ -80,3 +84,8 @@ export const leafColumnIds = ( | |||
|
|
|||
| export const isExperimentColumn = (id: string): boolean => | |||
| id === EXPERIMENT_COLUMN_ID | |||
|
|
|||
| export const isDefaultColumn = (id: string) => | |||
There was a problem hiding this comment.
[Q] Couldn't we just use DEFAULT_COLUMN_IDS instead of importing all of the individual IDs?
[Q] Won't the addition of these two columns break <WithExpColumnNeedsShadowUpdates>?
There was a problem hiding this comment.
[Q] Couldn't we just use DEFAULT_COLUMN_IDS instead of importing all of the individual IDs?
Yes, thanks for catching that!
[Q] Won't the addition of these two columns break ?
Forgot to mention this in a review comment, For now, the columns aren't sticky:
Screen.Recording.2023-09-27.at.6.53.38.AM.mov
I wanted to save it for a follow-up since I was having some difficulty making three columns sticky and have the experiment column conditionally have a shadow.
| @@ -108,7 +112,9 @@ export const ExperimentsTable: React.FC = () => { | |||
| toggleAllRowsExpanded() | |||
| }, [toggleAllRowsExpanded]) | |||
|
|
|||
| const hasOnlyDefaultColumns = columns.length <= 1 | |||
| const hasOnlyDefaultColumns = columns.every( | |||
| ({ id }) => id && isDefaultColumn(id) | |||
There was a problem hiding this comment.
[Q] Does every return true if the array is empty?
* rename variables * reuse header
main<= #4685 <= thisDemo
Screen.Recording.2023-09-26.at.12.12.39.PM.mov
Part of #4620