-
Notifications
You must be signed in to change notification settings - Fork 24
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
PORTALS-2651 - Download Cart #1536
PORTALS-2651 - Download Cart #1536
Conversation
nickgros
commented
Jan 30, 2025
- Remove react-bootstrap from DownloadCart
- Add column pinning styles to shared TanStack table UI components
- Fix some stale query / query key issues related to the download cart queries
- Remove react-bootstrap from DownloadCart - Add column pinning styles to shared TanStack table UI components - Fix some stale query / query key issues related to the download cart queries
@@ -306,10 +306,7 @@ export function DownloadCartPage(props: DownloadListActionsRequiredProps) { | |||
} | |||
numFiles={data.numberOfFilesAvailableForDownload} | |||
/> | |||
<AvailableForDownloadTable | |||
filesStatistics={data} | |||
refetchStatistics={refetch} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed refetchStatistics
and properly linked up the react-query hooks to invalidate the cache when statistics should be re-fetched
@@ -89,16 +83,14 @@ describe('DownloadListTable tests', () => { | |||
beforeEach(() => { | |||
jest.clearAllMocks() | |||
}) | |||
it('loads more available download files when inView', async () => { | |||
it('loads more available download files when "show more" is clicked', async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This had an infinite scroll, which was inappropriate for a non-height constrained table that could be very long. So I changed to a "Show More" pattern
- Update SynapseClient
|
||
function InteractiveSortIcon(props: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed this component, sort UI is now handled by common TanStack table components
columnSortBy: SortField | ||
sort: Sort | undefined | ||
setSort: React.Dispatch<React.SetStateAction<Sort | undefined>> | ||
function RemoveFromDownloadListButton(props: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Encapsulate RemoveFromDownloadListButton component to use useRemoveFilesFromDownloadList
hook (using useMutation
under-the-hood)
const columnHelper = createColumnHelper<DownloadListItemResult>() | ||
const getColumns = (args: { | ||
removeItem: (item: DownloadListItem, fileName: string, title: string) => void | ||
onCopySynIds: () => void | ||
}) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Create TanStack Table columns
// Load the next page when this ref comes into view. | ||
const { ref, inView } = useInView() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove useInView
and replace infinite scroll with "Show More" button
columnPinning: { | ||
right: ['actions'], | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TanStack table has built-in support for columnPinning
, which is basically the same thing as our static "Actions" column. The "pinned" column(s) will not change for this table, so this state value is constant.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice!
getRowClassNames(row: Row<DownloadListItemResult>) { | ||
if (!row.original.isEligibleForPackaging) { | ||
return 'ineligibleForPackaging' | ||
} | ||
return '' | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To properly set the background color the table cell, I am applying a class to the tr
element--the table cell itself has padding so I cannot simply apply the color to the cell contents.
<Select native fullWidth value={getFilterDisplayText(filter)}> | ||
{availableFiltersArray.map(availableFilter => ( | ||
<option | ||
key={`${getFilterDisplayText(availableFilter)}-filter-option`} | ||
onClick={() => { | ||
setFilter(availableFilter) | ||
}} | ||
> | ||
{getFilterDisplayText(availableFilter)} | ||
</option> | ||
))} | ||
</Select> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Replace Bootstrap Dropdown with MUI Select
{/* TODO: This table can be very large, so it should be refactored to use row virtualization or discrete pagination */} | ||
<StyledTanStackTable table={table} fullWidth={false} /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use common TanStack table UI. Left a note for future improvements
</tbody> | ||
</Table> | ||
</> | ||
{isFetchingNextPage ? 'Loading...' : 'Show More'} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added Show More button
@@ -106,6 +107,7 @@ export default function StyledTanStackTable< | |||
width: `calc(var(${getHeaderSizeCssVariable( | |||
header.id, | |||
)}) * 1px)`, | |||
...getCommonPinningStyles(header.column), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add general support for column pinning
let mergedClassNames = trSlotProps.className ?? '' | ||
if (table.options.meta?.getRowClassNames) { | ||
mergedClassNames = | ||
`${mergedClassNames} ${table.options.meta.getRowClassNames( | ||
row, | ||
)}`.trim() | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add general support for dynamically setting row class names based on row data
['tr']: { | ||
backgroundColor: theme.palette.background.default, | ||
}, | ||
['tr:nth-of-type(even)']: noStripedRows | ||
? undefined | ||
: { | ||
backgroundColor: theme.palette.grey[100], | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Explicitly set the background of the table rows, or else the pinned columns have transparent backgrounds
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't want to see the column content behind the pinned column? :D
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The opacity applies to the entire column including this background (at least on Firefox), so you can still see through it!
Just wanted to note that there will be merge conflicts with #1538 |
- Remove unused SCSS mixins
…b-monorepo into PORTALS-2651-download-table
<AvailableForDownloadTable | ||
filesStatistics={data} | ||
refetchStatistics={refetch} | ||
numBytes={data.sumOfFileSizesAvailableForDownload} | ||
numPackagableFiles={ | ||
data.numberOfFilesAvailableForDownloadAndEligibleForPackaging | ||
} | ||
numFiles={data.numberOfFilesAvailableForDownload} | ||
/> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove props, get this data directly from the query hook
))} | ||
</Dropdown.Menu> | ||
</Dropdown> | ||
<Select native fullWidth value={getFilterDisplayText(filter)}> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Replace bootstrap dropdown with MUI Select
|
||
const availableFiltersArray: AvailableFilter[] = [ | ||
undefined, | ||
'eligibleForPackaging', | ||
'ineligibleForPackaging', | ||
] | ||
return ( | ||
<div className="bootstrap-4-backport"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is one of my favorite lines from this PR ;)