Skip to content
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-2795 - Replace react-base-table usage with TanStack table #1486

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

nickgros
Copy link
Collaborator

@nickgros nickgros commented Jan 7, 2025

  • Update EntityFinder's DetailsView to use StyledVirtualTanStackTable
  • Update ChallengeDataDownload component to use EntityFinder components, remove custom components with slightly modified code
    • Updated DetailsView to support showing/hiding Add to Download Cart & Direct Download columns
  • Update DatasetItemsEditor to use StyledVirtualTanStackTable
    • Fixed memoization issue in useSet
    • Fixed broken test by explicitly refetching the dataset on successful update

Visual Changes

Component Old New
Entity Finder image image
Dataset Editor image image
ChallengeDataDownload image image

- Update EntityFinder's DetailsView to use StyledVirtualTanStackTable
- Update ChallengeDataDownload component to use EntityFinder components, remove custom components with slightly modified code
  - Updated DetailsView to support showing/hiding Add to Download Cart & Direct Download columns
- Update DatasetItemsEditor to use StyledVirtualTanStackTable
  - Fixed memoization issue in useSet
  - Fixed broken test by explicitly refetching the dataset on successful update
@nickgros nickgros added the chromatic Deploys the latest commit on this PR to Chromatic and runs snapshot tests label Jan 7, 2025
Comment on lines +36 to +41
const [sortingState, setSortingState] = useState<SortingState>([
{
id: DetailsViewColumn.NAME,
desc: false,
},
])
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use the TanStack table sorting API in the state variable to make it easy to pass along

Comment on lines +197 to +198
expect(rows[FILE_INDEX + 1]).not.toBeDisabled()
expect(rows[PROJECT_INDEX + 1]).not.toBeDisabled()
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The table headers are now resolved as a row, so in this test suite, we need to increment all data indices by 1 to get the corresponding rendered row

Comment on lines +84 to +85
enableSorting?: boolean
enableMultiSort?: boolean
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use TanStack table sorting API so we don't need to do any additional transforms

noResultsPlaceholder?: ReactElement
enableSorting?: boolean
enableMultiSort?: boolean
sortableColumns?: DetailsViewColumn[]
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This table may get its data from different APIs, which may vary by which columns you can ask the server to sort results. If columns are sortable, the wrapping component must indicate which columns can be sorted.

@@ -72,6 +72,7 @@ export default function StyledVirtualTanStackTable<T = unknown>(
rowTransform={row => table.getRowModel().rows[row.index]}
fullWidth={fullWidth}
styledTableContainerProps={{
noStripedRows: true,
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Virtualized tables should not be striped

Comment on lines -69 to -74
$table-prefix: DatasetEditorTable;
$table-font-size: 14px;
$header-background-color: map.get(SRC.$colors, 'gray-200');
$row-hovered-background-color: unset;
$table-padding-left: 5px;
@import '@sage-bionetworks/react-base-table/es/BaseTable';
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All for react-base-table, removed

Comment on lines +30 to +36
td > * {
max-width: 100%;
text-overflow: ellipsis;
white-space: nowrap;
overflow: hidden;
display: block;
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do not let text overflow in table cells (needed, probably because we are now using CSS grid)

Comment on lines +40 to 48
// Select odd rows (data-index ends in an odd number)
// Don't use nth child because the virtual rows can switch indicies, causing style to flip for the same cell
&[data-index$='1'],
&[data-index$='3'],
&[data-index$='5'],
&[data-index$='7'],
&[data-index$='9'] {
background: SRC.$background-color-gray;
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The entity finder table is striped. Instead of trying to add a classname to every other tr, we can add the virtual row index to every tr, and change the background color if the attribute is odd (selector matches if the attribute string ends with '1','3','5','7','9')

}
setSet(newSet)
}
const add = useCallback(
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caused memoization issues with TanStack Table in the dataset editor

Comment on lines +74 to +80
hiddenColumns: [
DetailsViewColumn.BADGES,
DetailsViewColumn.ADD_TO_DOWNLOAD_CART,
DetailsViewColumn.CREATED_ON,
DetailsViewColumn.VERSION,
DetailsViewColumn.MODIFIED_BY,
],
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the challenge portal version of this component, hide these columns (instead of maintaining tons of duplicate code!).

- Typechecking for EntityBadgeIcons tests
- Move table cell components to individual files
Comment on lines +61 to +62
/** Chosen columns to hide. Defaults to just the DirectDownload column. */
hiddenColumns?: DetailsViewColumn[]
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exposed for challenge portal customization

[DetailsViewColumn.DIRECT_DOWNLOAD]: visibleTypes.includes(
EntityType.FILE,
),
...hideColumnOverrides,
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Columns passed via hiddenColumns prop will pass along show/hide state to useReactTable here

@@ -1,43 +1,40 @@
import { act, render, screen, waitFor } from '@testing-library/react'
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No logical changes to these tests. An unused prop was removed from the component under test. I moved this test file to the src folder and fixed type errors.

Comment on lines -71 to -72
/** Whether this component should render a ReactTooltip or if an external component is managing it */
renderTooltipComponent: boolean
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unused prop that dates back to when we used the react-tooltip library.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved all of these components to their own files, and modified them to use the TanStack table API. Minimal changes to rendered content (e.g. removed a wrapper div in a few places)

import { displayToast } from '../../../../ToastMessage/index'
import { EntityFinderTableViewRowData } from '../DetailsView'

export function AddFileToDownloadListCell(
Copy link
Collaborator Author

@nickgros nickgros Jan 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All of these table cell data components were moved into their own files to keep them simple & optimized for HMR.

They were changed to use the TanStack table API (which TypeScript will catch), but otherwise should not have many changes, so I don't think they need to be closely reviewed.

Comment on lines +143 to +147
const user = userEvent.setup()

const mockOnUnsavedChangesFn = jest.fn()
const mockOnSaveFn = jest.fn()
const mockOnCloseFn = jest.fn()
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed some flaky test issues by creating new mocks/userEvent instance each render. I think I had an issue where some actions in tests would fire after the test had ended and next test started, which made mock invocation counts incorrect.

Comment on lines +791 to +802
<Tooltip title={!hasChangedSinceLastSave && NO_CHANGES_MADE}>
<div>
<Button
disabled={!datasetToUpdate || !hasChangedSinceLastSave}
variant="contained"
color="primary"
onClick={() => mutate(datasetToUpdate!)}
>
Save
</Button>
</div>
</Tooltip>
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update 'Save' button to only enable when changes are pending. Add tooltip + div wrapper to tell the user why it's disabled.

Comment on lines +26 to +28
['thead > tr']: {
backgroundColor: theme.palette.grey[200],
},
Copy link
Collaborator Author

@nickgros nickgros Jan 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In addition to the th cells, add a background color to the entire header row. This prevents a visual regression where the full table width is wider than the sum of all columns.

react-base-table auto-sized the columns to take up at least the full width of the table container, so this wasn't an issue before.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chromatic Deploys the latest commit on this PR to Chromatic and runs snapshot tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant