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

feat(listbox): virtualization #4206

Merged
merged 13 commits into from
Dec 1, 2024
4 changes: 4 additions & 0 deletions apps/docs/content/components/listbox/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@ import description from "./description";
import sections from "./sections";
import customStyles from "./custom-styles";
import topContent from "./top-content";
import virtualization from "./virtualization";
import virtualizationTenThousand from "./virtualization-ten-thousand";

export const listboxContent = {
usage,
Expand All @@ -22,4 +24,6 @@ export const listboxContent = {
sections,
customStyles,
topContent,
virtualization,
virtualizationTenThousand,
};
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
import {Listbox, ListboxItem} from "@nextui-org/react";

const generateItems = (n) => {
const items = [
"Cat",
"Dog",
"Elephant",
"Lion",
"Tiger",
"Giraffe",
"Dolphin",
"Penguin",
"Zebra",
"Shark",
"Whale",
"Otter",
"Crocodile",
];

const dataset = [];

for (let i = 0; i < n; i++) {
const item = items[i % items.length];

dataset.push({
label: `${item}${i}`,
value: `${item.toLowerCase()}${i}`,
description: "Sample description",
});
}

return dataset;
};
Comment on lines +3 to +33
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Add input validation and improve memory efficiency

The generateItems function could benefit from several improvements:

  1. Add input validation for the n parameter
  2. Consider chunking or lazy loading for large datasets
  3. Add more variety to descriptions for better testing

Consider this improved implementation:

-const generateItems = (n) => {
+const generateItems = (n: number) => {
+  if (!Number.isInteger(n) || n < 0) {
+    throw new Error('n must be a positive integer');
+  }
+
   const items = [
     "Cat",
     "Dog",
     // ... other items
   ];
+
+  const descriptions = [
+    "Very friendly",
+    "Playful and energetic",
+    "Gentle giant",
+    "King of the jungle",
+  ];

   const dataset = [];

   for (let i = 0; i < n; i++) {
     const item = items[i % items.length];
+    const description = descriptions[i % descriptions.length];

     dataset.push({
       label: `${item}${i}`,
       value: `${item.toLowerCase()}${i}`,
-      description: "Sample description",
+      description,
     });
   }

   return dataset;
 };

Committable suggestion skipped: line range outside the PR's diff.


export default function App() {
const items = generateItems(1000);

return (
<div className="flex w-full flex-wrap md:flex-nowrap gap-4">
<Listbox
isVirtualized
label={"Select from 10000 items"}
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Fix incorrect item count in label

The label mentions "10000 items" but the component only generates 1000 items.

-label={"Select from 10000 items"}
+label={"Select from 1000 items"}
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
label={"Select from 10000 items"}
label={"Select from 1000 items"}

placeholder="Select..."
virtualization={{
maxListboxHeight: 400,
itemHeight: 40,
}}
>
{items.map((item, index) => (
<ListboxItem key={index} value={item.value}>
{item.label}
</ListboxItem>
))}
Comment on lines +49 to +53
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Avoid using array index as React key

Using array indices as keys can lead to performance issues and bugs with item state when the list is modified. Since each item has a unique value, use that instead.

-<ListboxItem key={index} value={item.value}>
+<ListboxItem key={item.value} value={item.value}>
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
{items.map((item, index) => (
<ListboxItem key={index} value={item.value}>
{item.label}
</ListboxItem>
))}
{items.map((item, index) => (
<ListboxItem key={item.value} value={item.value}>
{item.label}
</ListboxItem>
))}

</Listbox>
</div>
);
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
import App from "./virtualization-ten-thousand.raw.jsx?raw";

const react = {
"/App.jsx": App,
};

export default {
...react,
};
56 changes: 56 additions & 0 deletions apps/docs/content/components/listbox/virtualization.raw.jsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
import {Listbox, ListboxItem} from "@nextui-org/react";
const generateItems = (n) => {
const items = [
"Cat",
"Dog",
"Elephant",
"Lion",
"Tiger",
"Giraffe",
"Dolphin",
"Penguin",
"Zebra",
"Shark",
"Whale",
"Otter",
"Crocodile",
];

const dataset = [];

for (let i = 0; i < n; i++) {
const item = items[i % items.length];

dataset.push({
label: `${item}${i}`,
value: `${item.toLowerCase()}${i}`,
description: "Sample description",
});
}

return dataset;
};

export default function App() {
const items = generateItems(1000);

return (
<div className="flex w-full flex-wrap md:flex-nowrap gap-4">
<Listbox
isVirtualized
label={"Select from 1000 items"}
placeholder="Select..."
virtualization={{
maxListboxHeight: 400,
itemHeight: 40,
}}
>
{items.map((item, index) => (
<ListboxItem key={index} value={item.value}>
{item.label}
</ListboxItem>
))}
</Listbox>
</div>
);
}
Comment on lines +34 to +56
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Address potential performance and accessibility concerns

Several improvements could enhance the component's robustness:

  1. Using index as key in the map function could cause issues with React's reconciliation
  2. Consider making the virtualization dimensions responsive
  3. Add proper aria labels for accessibility

Consider these improvements:

 export default function App() {
   const items = generateItems(1000);
+  const containerRef = useRef(null);
+  const [maxHeight, setMaxHeight] = useState(400);
+
+  useEffect(() => {
+    const updateHeight = () => {
+      if (containerRef.current) {
+        setMaxHeight(window.innerHeight * 0.6);
+      }
+    };
+    window.addEventListener('resize', updateHeight);
+    updateHeight();
+    return () => window.removeEventListener('resize', updateHeight);
+  }, []);

   return (
-    <div className="flex w-full flex-wrap md:flex-nowrap gap-4">
+    <div ref={containerRef} className="flex w-full flex-wrap md:flex-nowrap gap-4">
       <Listbox
         isVirtualized
+        aria-label="Select an animal"
         label={"Select from 1000 items"}
         placeholder="Select..."
         virtualization={{
-          maxListboxHeight: 400,
+          maxListboxHeight: maxHeight,
           itemHeight: 40,
         }}
       >
         {items.map((item, index) => (
-          <ListboxItem key={index} value={item.value}>
+          <ListboxItem key={item.value} value={item.value}>
             {item.label}
           </ListboxItem>
         ))}
       </Listbox>
     </div>
   );
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
export default function App() {
const items = generateItems(1000);
return (
<div className="flex w-full flex-wrap md:flex-nowrap gap-4">
<Listbox
isVirtualized
label={"Select from 1000 items"}
placeholder="Select..."
virtualization={{
maxListboxHeight: 400,
itemHeight: 40,
}}
>
{items.map((item, index) => (
<ListboxItem key={index} value={item.value}>
{item.label}
</ListboxItem>
))}
</Listbox>
</div>
);
}
export default function App() {
const items = generateItems(1000);
const containerRef = useRef(null);
const [maxHeight, setMaxHeight] = useState(400);
useEffect(() => {
const updateHeight = () => {
if (containerRef.current) {
setMaxHeight(window.innerHeight * 0.6);
}
};
window.addEventListener('resize', updateHeight);
updateHeight();
return () => window.removeEventListener('resize', updateHeight);
}, []);
return (
<div ref={containerRef} className="flex w-full flex-wrap md:flex-nowrap gap-4">
<Listbox
isVirtualized
aria-label="Select an animal"
label={"Select from 1000 items"}
placeholder="Select..."
virtualization={{
maxListboxHeight: maxHeight,
itemHeight: 40,
}}
>
{items.map((item, index) => (
<ListboxItem key={item.value} value={item.value}>
{item.label}
</ListboxItem>
))}
</Listbox>
</div>
);
}

9 changes: 9 additions & 0 deletions apps/docs/content/components/listbox/virtualization.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
import App from "./virtualization.raw.jsx?raw";

const react = {
"/App.jsx": App,
};

export default {
...react,
};
28 changes: 28 additions & 0 deletions apps/docs/content/docs/components/listbox.mdx
Original file line number Diff line number Diff line change
Expand Up @@ -151,6 +151,22 @@ function App() {
}
```

### Virtualization

Select supports virtualization, which allows efficient rendering of large lists by only rendering items that are visible in the viewport. You can enable virtualization by setting the `isVirtualized` prop to `true`.

<CodeDemo
title="Virtualization"
files={listboxContent.virtualization}
/>

> **Note**: The virtualization strategy is based on the [@tanstack/react-virtual](https://tanstack.com/virtual/latest) package, which provides efficient rendering of large lists by only rendering items that are visible in the viewport.
#### Ten Thousand Items

Here's an example of using virtualization with 10,000 items.

<CodeDemo title="Ten Thousand Items" files={listboxContent.virtualizationTenThousand} />

## Slots

Listbox has 3 components with slots the base one `Listbox`, `ListboxItem` and `ListboxSection` components.
Expand Down Expand Up @@ -328,6 +344,18 @@ You can customize the `Listbox` items style by using the `itemClasses` prop and
type: "boolean",
description: "Whether keyboard navigation is circular.",
default: "false"
},
{
attribute: "isVirtualized",
type: "boolean",
description: "Whether to enable virtualization.",
default: "false"
},
{
attribute: "virtualization",
type: "Record<\"maxListboxHeight\" & \"itemHeight\", number>",
description: "Configuration for virtualization, optimizing rendering for large datasets. Required if isVirtualized is set to true.",
default: "-",
Comment on lines +349 to +358
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Correct Type Definition for 'virtualization' Prop

The type for the virtualization prop is specified as Record<"maxListboxHeight" & "itemHeight", number>, but the intersection of two string literals results in never, which is not intended. To properly define an object with both maxListboxHeight and itemHeight properties, specify it as:

Apply this diff to correct the type definition:

-          type: "Record<\"maxListboxHeight\" & \"itemHeight\", number>",
+          type: "{ maxListboxHeight: number; itemHeight: number }",
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
attribute: "isVirtualized",
type: "boolean",
description: "Whether to enable virtualization.",
default: "false"
},
{
attribute: "virtualization",
type: "Record<\"maxListboxHeight\" & \"itemHeight\", number>",
description: "Configuration for virtualization, optimizing rendering for large datasets. Required if isVirtualized is set to true.",
default: "-",
attribute: "isVirtualized",
type: "boolean",
description: "Whether to enable virtualization.",
default: "false"
},
{
attribute: "virtualization",
type: "{ maxListboxHeight: number; itemHeight: number }",
description: "Configuration for virtualization, optimizing rendering for large datasets. Required if isVirtualized is set to true.",
default: "-",

},
{
attribute: "hideEmptyContent",
Expand Down
105 changes: 105 additions & 0 deletions packages/components/listbox/stories/listbox.stories.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -679,6 +679,59 @@ const CustomWithClassNamesTemplate = ({color, variant, disableAnimation, ...args
);
};

interface LargeDatasetSchema {
label: string;
value: string;
description: string;
}

function generateLargeDataset(n: number): LargeDatasetSchema[] {
const dataset: LargeDatasetSchema[] = [];
const items = [
"Cat",
"Dog",
"Elephant",
"Lion",
"Tiger",
"Giraffe",
"Dolphin",
"Penguin",
"Zebra",
"Shark",
"Whale",
"Otter",
"Crocodile",
];

for (let i = 0; i < n; i++) {
const item = items[i % items.length];

dataset.push({
label: `${item}${i}`,
value: `${item.toLowerCase()}${i}`,
description: "Sample description",
});
}

return dataset;
}

const LargeDatasetTemplate = (args: ListboxProps & {numItems: number}) => {
const largeDataset = generateLargeDataset(args.numItems);

return (
<div className="flex w-full max-w-full py-20 px-20">
<Listbox label={`Select from ${args.numItems} items`} {...args}>
{largeDataset.map((item, index) => (
<ListboxItem key={index} value={item.value}>
{item.label}
</ListboxItem>
Comment on lines +726 to +728
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Use Stable Keys for List Items

In the largeDataset.map(...), you're using index as the key for each ListboxItem. Using indices as keys can lead to issues with component state and rendering when the list changes. It's recommended to use a unique and stable identifier for keys, such as item.value.

Apply this diff to use stable keys:

-        {largeDataset.map((item, index) => (
-          <ListboxItem key={index} value={item.value}>
+        {largeDataset.map((item) => (
+          <ListboxItem key={item.value} value={item.value}>
             {item.label}
           </ListboxItem>
         ))}

Committable suggestion skipped: line range outside the PR's diff.

))}
</Listbox>
</div>
);
};

export const Default = {
render: Template,
args: {
Expand Down Expand Up @@ -782,3 +835,55 @@ export const CustomWithClassNames = {
...defaultProps,
},
};

export const OneThousandList = {
render: LargeDatasetTemplate,
args: {
...defaultProps,
numItems: 1000,
isVirtualized: true,
virtualization: {
maxListboxHeight: 400,
itemHeight: 20,
},
},
};

export const TenThousandList = {
render: LargeDatasetTemplate,
args: {
...defaultProps,
numItems: 10000,
isVirtualized: true,
virtualization: {
maxListboxHeight: 400,
itemHeight: 20,
},
},
};

export const CustomMaxListboxHeight = {
render: LargeDatasetTemplate,
args: {
...defaultProps,
numItems: 1000,
isVirtualized: true,
virtualization: {
maxListboxHeight: 600,
itemHeight: 20,
},
},
};

export const CustomItemHeight = {
render: LargeDatasetTemplate,
args: {
...defaultProps,
numItems: 1000,
isVirtualized: true,
virtualization: {
itemHeight: 40,
maxListboxHeight: 600,
},
},
};
2 changes: 1 addition & 1 deletion pnpm-lock.yaml

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

Loading