fix(storage-browser): set up location detail view table using data table #5726
Conversation
|
| className: `${TABLE_DATA_CLASS_NAME} ${TABLE_DATA_CLASS_NAME}--download`, | ||
| key: `td-download-${index}`, | ||
| children: ( | ||
| <SpanElement className={TABLE_DATA_TEXT_CLASS_NAME}> |
There was a problem hiding this comment.
This SpanElement wrapping the DownloadControl can be removed (the button styles on Download already supply the padding/string truncation that plain text cells would otherwise need).
There was a problem hiding this comment.
got it, thanks Heather. Just pushed up the change to remove the span
| <LocationDetailViewTable /> | ||
| <DataTableControl /> |
There was a problem hiding this comment.
Can we delete LocationDetailViewTable with this change?
There was a problem hiding this comment.
yes we can, was waiting for Heather's changes for pagination to get in first
packages/react-storage/src/components/StorageBrowser/Views/utils.ts
Outdated
Show resolved
Hide resolved
packages/react-storage/src/components/StorageBrowser/components/DataTable.tsx
Outdated
Show resolved
Hide resolved
| }; | ||
|
|
||
| // Generate the data for each row in the table | ||
| const getLocationsItemData = ({ |
There was a problem hiding this comment.
Bit concerned abt the size of this utility, feels like there should be a separation of concerns
| <ButtonElement variant="sort" className={TABLE_HEADER_BUTTON_CLASS_NAME}> | ||
| {header} | ||
| <IconElement | ||
| variant={ | ||
| selection === key && direction !== 'none' | ||
| ? `sort-${direction}` | ||
| : 'sort-indeterminate' | ||
| } | ||
| /> | ||
| </ButtonElement> | ||
| ), |
There was a problem hiding this comment.
Shouldn't this logic be handled by renderColumnHeaderItem?
| export type SortState = { | ||
| selection: string; | ||
| direction: SortDirection; | ||
| }; | ||
|
|
||
| type Column<T> = { | ||
| key: keyof T; | ||
| header: string; | ||
| compareFn?: (a: T, b: T) => number; | ||
| }; | ||
|
|
||
| type DataTableLocationItems = { | ||
| key: string; | ||
| fileExt: string; | ||
| lastModified: Date | undefined; | ||
| size: number | undefined; | ||
| type: string; | ||
| }; |
There was a problem hiding this comment.
Few things:
- these should be declared as interfaces, not types
- why are we reintroducing a
Columninterface? This isn't needed for theLocationsViewright?
There was a problem hiding this comment.
added it to help with managing the key and header properties since I wanted to add a new column, fileExt that would contain the file extensions, but display as as a type column.
I think when I tried to set it up similar to how LocationsView was set up, I was running into a lot of typescript errors related to the sorting, but I can try it out again
…ta-table-locationDetailViewTable
Description of changes
Issue #, if available
Description of how you validated changes
Checklist
yarn testpasses and tests are updated/addeddocs,e2e,examples, or other private packages.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.