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.

Sign up for GitHub

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

Added virtualised list to marketplace for quicker model rendering #1978

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Added virtualised list to marketplace for quicker model rendering #1978

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 24 additions & 0 deletions frontend/package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions frontend/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@
"react": "18.3.1",
"react-dom": "18.3.1",
"react-to-print": "^2.15.1",
"react-window": "^1.8.11",
"rehype-highlight": "^7.0.1",
"remark-frontmatter": "^5.0.0",
"remark-gfm": "^4.0.0",
Expand Down
93 changes: 57 additions & 36 deletions frontend/src/marketplace/EntryList.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
import { Box, Stack, Typography } from '@mui/material'
import { useTheme } from '@mui/material/styles'
import { EntrySearchResult } from 'actions/model'
import { Fragment, useMemo } from 'react'
import { CSSProperties, memo } from 'react'
import { FixedSizeList } from 'react-window'
import ChipSelector from 'src/common/ChipSelector'
import EmptyBlob from 'src/common/EmptyBlob'
import Link from 'src/Link'
Expand All @@ -22,49 +23,69 @@ export default function EntryList({
}: EntryListProps) {
const theme = useTheme()

const entriesDisplay = useMemo(() => {
return entries.map((entry, index) => (
<Fragment key={entry.id}>
<Stack direction='row'>
<Link
sx={{ textDecoration: 'none', whiteSpace: 'nowrap', textOverflow: 'ellipsis', overflow: 'hidden' }}
href={`${entry.kind}/${entry.id}`}
>
<Typography
variant='h5'
sx={{ fontWeight: '500', textDecoration: 'none', color: theme.palette.primary.main }}
const Row = memo(({ data, index, style }: { data: EntrySearchResult[]; index: number; style: CSSProperties }) => {
Copy link
Member

Choose a reason for hiding this comment

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

If you want to use memo(), which is a HOC (Higher Order Component), you should split this out into its own component.

That being said, I don't really see the performance benefit of memoizing something that's going to be unrendered and rerendered as the user scrolls.

IMO this should be pulled out into a new EntryListRow component and we can do away with the memo. I do think it's worth checking that there isn't a degradation of performance if we do this, but I wouldn't expect there to be any noticeable change either way. Therefore I'd vote for simplifying by removing the memo.

const entry = data[index]

return (
<Box
justifyContent='flex-start'
alignItems='center'
sx={{ backgroundColor: index % 2 ? theme.palette.container.main : 'none', p: 2, margin: 'auto', ...style }}
Copy link
Member

Choose a reason for hiding this comment

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

Now that we've got a scroll bar to the right and have also changed how the container for each list item looks, I'd be quite keen to reduce/remove the horizontal padding we have on this list and the tabs above it.

key={entry.id}
>
<Stack>
<Stack direction='row'>
<Link
sx={{ textDecoration: 'none', whiteSpace: 'nowrap', textOverflow: 'ellipsis', overflow: 'hidden' }}
href={`${entry.kind}/${entry.id}`}
>
{entry.name}
</Typography>
</Link>
<Typography
variant='h5'
sx={{ fontWeight: '500', textDecoration: 'none', color: theme.palette.primary.main }}
>
{entry.name}
</Typography>
</Link>
</Stack>
<Typography
variant='body1'
sx={{ marginBottom: 2, whiteSpace: 'nowrap', textOverflow: 'ellipsis', overflow: 'hidden' }}
>
{entry.description}
</Typography>
<div>
Copy link
Member

Choose a reason for hiding this comment

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

Is this div needed?

<ChipSelector
chipTooltipTitle={'Filter by tag'}
options={entry.tags.slice(0, 10)}
expandThreshold={10}
multiple
selectedChips={selectedChips}
onChange={onSelectedChipsChange}
size='small'
ariaLabel='add tag to search filter'
/>
</div>
</Stack>
<Typography
variant='body1'
sx={{ marginBottom: 2, whiteSpace: 'nowrap', textOverflow: 'ellipsis', overflow: 'hidden' }}
>
{entry.description}
</Typography>
<ChipSelector
chipTooltipTitle={'Filter by tag'}
options={entry.tags.slice(0, 10)}
expandThreshold={10}
multiple
selectedChips={selectedChips}
onChange={onSelectedChipsChange}
size='small'
ariaLabel='add tag to search filter'
/>
{index !== entries.length - 1 && <Box sx={{ borderBottom: 1, borderColor: 'divider', marginBottom: 2 }} />}
</Fragment>
))
}, [entries, onSelectedChipsChange, selectedChips, theme.palette.primary.main])
</Box>
)
})

Row.displayName = 'row'

if (entriesErrorMessage) return <MessageAlert message={entriesErrorMessage} severity='error' />

return (
<>
{entries.length === 0 && <EmptyBlob data-test='emptyEntryListBlob' text='No entries here' />}
{entriesDisplay}
<FixedSizeList
Copy link
Member

Choose a reason for hiding this comment

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

From past experience setting overscanCount to something like 5 or 10 helps to reduce stutter when scrolling very quickly

height={window.innerHeight - 300}
itemCount={entries.length}
itemData={entries}
itemSize={130}
width='100%'
>
{Row}
</FixedSizeList>
</>
)
}
28 changes: 28 additions & 0 deletions package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading