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

fix+feat(select, listbox): bug on dataset with "sections", add support for scrollshadow #4462

Merged
merged 4 commits into from
Jan 2, 2025
Merged

fix+feat(select, listbox): bug on dataset with "sections", add support for scrollshadow #4462

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
30b1832
fix: add custom function to calculate rowHeight for dataset with sect…
vinroger Dec 30, 2024
c2e9459
fix: scroll shadow is now working in virtualized components
vinroger Dec 30, 2024
848863f
chore: add changeset
vinroger Dec 30, 2024
86dcb3d
fix: to pass test cases use function call instead of function component
vinroger Dec 30, 2024
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
7 changes: 7 additions & 0 deletions .changeset/quiet-geese-lay.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
---
"@nextui-org/autocomplete": patch
"@nextui-org/listbox": patch
"@nextui-org/select": patch
---

add support for dataset with section, add support for scrollshadow
1 change: 1 addition & 0 deletions packages/components/autocomplete/src/use-autocomplete.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -461,6 +461,7 @@ export function useAutocomplete<T extends object>(originalProps: UseAutocomplete
itemHeight,
}
: undefined,
scrollShadowProps: slotsProps.scrollShadowProps,
...mergeProps(slotsProps.listboxProps, listBoxProps, {
shouldHighlightOnFocus: true,
}),
Expand Down
109 changes: 91 additions & 18 deletions packages/components/listbox/src/virtualized-listbox.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,10 @@
import {useRef} from "react";
import {useMemo, useRef, useState} from "react";
import {mergeProps} from "@react-aria/utils";
import {useVirtualizer} from "@tanstack/react-virtual";
import {isEmpty} from "@nextui-org/shared-utils";
import {Node} from "@react-types/shared";
import {ScrollShadowProps, useScrollShadow} from "@nextui-org/scroll-shadow";
import {filterDOMProps} from "@nextui-org/react-utils";

import ListboxItem from "./listbox-item";
import ListboxSection from "./listbox-section";
Expand All @@ -11,8 +14,50 @@ import {UseListboxReturn} from "./use-listbox";
interface Props extends UseListboxReturn {
isVirtualized?: boolean;
virtualization?: VirtualizationProps;
/* Here in virtualized listbox, scroll shadow needs custom implementation. Hence this is the only way to pass props to scroll shadow */
scrollShadowProps?: Partial<ScrollShadowProps>;
}

const getItemSizesForCollection = (collection: Node<object>[], itemHeight: number) => {
const sizes: number[] = [];

for (const item of collection) {
if (item.type === "section") {
/* +1 for the section header */
sizes.push(([...item.childNodes].length + 1) * itemHeight);
} else {
sizes.push(itemHeight);
}
}

return sizes;
};

const getScrollState = (element: HTMLDivElement | null) => {
if (
!element ||
element.scrollTop === undefined ||
element.clientHeight === undefined ||
element.scrollHeight === undefined
) {
return {
isTop: false,
isBottom: false,
isMiddle: false,
};
}

const isAtTop = element.scrollTop === 0;
const isAtBottom = Math.ceil(element.scrollTop + element.clientHeight) >= element.scrollHeight;
const isInMiddle = !isAtTop && !isAtBottom;

return {
isTop: isAtTop,
isBottom: isAtBottom,
isMiddle: isInMiddle,
};
};

const VirtualizedListbox = (props: Props) => {
const {
Component,
Expand All @@ -29,6 +74,7 @@ const VirtualizedListbox = (props: Props) => {
disableAnimation,
getEmptyContentProps,
getListProps,
scrollShadowProps,
} = props;

const {virtualization} = props;
Expand All @@ -45,17 +91,24 @@ const VirtualizedListbox = (props: Props) => {

const listHeight = Math.min(maxListboxHeight, itemHeight * state.collection.size);

const parentRef = useRef(null);
const parentRef = useRef<HTMLDivElement>(null);
const itemSizes = useMemo(
() => getItemSizesForCollection([...state.collection], itemHeight),
[state.collection, itemHeight],
);

const rowVirtualizer = useVirtualizer({
count: state.collection.size,
count: [...state.collection].length,
getScrollElement: () => parentRef.current,
estimateSize: () => itemHeight,
estimateSize: (i) => itemSizes[i],
});

const virtualItems = rowVirtualizer.getVirtualItems();

const renderRow = ({
/* Here we need the base props for scroll shadow, contains the className (scrollbar-hide and scrollshadow config based on the user inputs on select props) */
const {getBaseProps: getBasePropsScrollShadow} = useScrollShadow({...scrollShadowProps});

const ListBoxRow = ({
index,
style: virtualizerStyle,
}: {
Expand All @@ -64,6 +117,10 @@ const VirtualizedListbox = (props: Props) => {
}) => {
const item = [...state.collection][index];

if (!item) {
return null;
}

const itemProps = {
color,
item,
Expand Down Expand Up @@ -102,6 +159,12 @@ const VirtualizedListbox = (props: Props) => {
return listboxItem;
};

const [scrollState, setScrollState] = useState({
isTop: false,
isBottom: true,
isMiddle: false,
});

const content = (
<Component {...getListProps()}>
{!state.collection.size && !hideEmptyContent && (
Expand All @@ -110,11 +173,18 @@ const VirtualizedListbox = (props: Props) => {
</li>
)}
<div
{...filterDOMProps(getBasePropsScrollShadow())}
ref={parentRef}
data-bottom-scroll={scrollState.isTop}
data-top-bottom-scroll={scrollState.isMiddle}
data-top-scroll={scrollState.isBottom}
style={{
height: maxListboxHeight,
overflow: "auto",
}}
onScroll={(e) => {
setScrollState(getScrollState(e.target as HTMLDivElement));
}}
>
{listHeight > 0 && itemHeight > 0 && (
<div
Expand All @@ -124,19 +194,22 @@ const VirtualizedListbox = (props: Props) => {
position: "relative",
}}
>
{virtualItems.map((virtualItem) =>
renderRow({
index: virtualItem.index,
style: {
position: "absolute",
top: 0,
left: 0,
width: "100%",
height: `${virtualItem.size}px`,
transform: `translateY(${virtualItem.start}px)`,
},
}),
)}
{virtualItems.map((virtualItem) => {
return (
<ListBoxRow
key={virtualItem.index}
index={virtualItem.index}
style={{
position: "absolute",
top: 0,
left: 0,
width: "100%",
height: `${virtualItem.size}px`,
transform: `translateY(${virtualItem.start}px)`,
}}
/>
);
})}
</div>
)}
</div>
Expand Down
5 changes: 4 additions & 1 deletion packages/components/select/src/use-select.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,7 +161,9 @@ export type UseSelectProps<T> = Omit<
SelectVariantProps & {
/**
* The height of each item in the listbox.
* For dataset with sections, the itemHeight must be the height of each item (including padding, border, margin).
* This is required for virtualized listboxes to calculate the height of each item.
* @default 36
*/
itemHeight?: number;
/**
Expand Down Expand Up @@ -208,7 +210,7 @@ export function useSelect<T extends object>(originalProps: UseSelectProps<T>) {
onSelectionChange,
placeholder,
isVirtualized,
itemHeight = 32,
itemHeight = 36,
maxListboxHeight = 256,
children,
disallowEmptySelection = false,
Expand Down Expand Up @@ -564,6 +566,7 @@ export function useSelect<T extends object>(originalProps: UseSelectProps<T>) {
className: slots.listbox({
class: clsx(classNames?.listbox, props?.className),
}),
scrollShadowProps: slotsProps.scrollShadowProps,
...mergeProps(slotsProps.listboxProps, props, menuProps),
} as ListboxProps;
};
Expand Down
98 changes: 98 additions & 0 deletions packages/components/select/stories/select.stories.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1391,6 +1391,104 @@ export const CustomItemHeight = {
},
};

const AVATAR_DECORATIONS: {[key: string]: string[]} = {
arcane: ["jinx", "atlas-gauntlets", "flame-chompers", "fishbones", "hexcore", "shimmer"],
anime: ["cat-ears", "heart-bloom", "in-love", "in-tears", "soul-leaving-body", "starry-eyed"],
"lofi-vibes": ["chromawave", "cozy-cat", "cozy-headphones", "doodling", "rainy-mood"],
valorant: [
"a-hint-of-clove",
"blade-storm",
"cypher",
"frag-out",
"omen-cowl",
"reyna-leer",
"vct-supernova",
"viper",
"yoru",
"carnalito2",
"a-hint-of-clove2",
"blade-storm2",
"cypher2",
"frag-out2",
"omen-cowl2",
"reyna-leer2",
"vct-supernova2",
"viper2",
"yoru2",
"carnalito3",
"a-hint-of-clove3",
"blade-storm3",
"cypher3",
"frag-out3",
"omen-cowl3",
"reyna-leer3",
"vct-supernova3",
"viper3",
"yoru3",
"carnalito4",
"a-hint-of-clove4",
"blade-storm4",
"cypher4",
"frag-out4",
"omen-cowl4",
"reyna-leer4",
"vct-supernova4",
"viper4",
"yoru4",
],
spongebob: [
"flower-clouds",
"gary-the-snail",
"imagination",
"musclebob",
"sandy-cheeks",
"spongebob",
],
arcade: ["clyde-invaders", "hot-shot", "joystick", "mallow-jump", "pipedream", "snake"],
"street-fighter": ["akuma", "cammy", "chun-li", "guile", "juri", "ken", "m.bison", "ryu"],
};

export const NonVirtualizedVsVirtualizedWithSections = {
render: () => {
const SelectComponent = ({isVirtualized}: {isVirtualized: boolean}) => (
<Select
disallowEmptySelection
className="max-w-xs"
color="secondary"
defaultSelectedKeys={["jinx"]}
isVirtualized={isVirtualized}
label={`Avatar Decoration ${isVirtualized ? "(Virtualized)" : "(Non-virtualized)"}`}
selectedKeys={["jinx"]}
selectionMode="single"
variant="bordered"
>
{Object.keys(AVATAR_DECORATIONS).map((key) => (
<SelectSection
key={key}
classNames={{
heading: "uppercase text-secondary",
}}
title={key}
>
{AVATAR_DECORATIONS[key].map((item) => (
<SelectItem key={item} className="capitalize" color="secondary" variant="bordered">
{item.replace(/-/g, " ")}
</SelectItem>
))}
</SelectSection>
))}
</Select>
);

return (
<div className="flex gap-4 w-full">
<SelectComponent isVirtualized={false} />
<SelectComponent isVirtualized={true} />
</div>
);
},
};
Comment on lines +1451 to +1490
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

Enhance component implementation for better accessibility and performance

The implementation could benefit from several improvements:

  1. The hardcoded selectedKeys={["jinx"]} duplicates the defaultSelectedKeys prop
  2. Missing aria-label for better screen reader support
  3. No error handling for invalid selections

Consider these improvements:

 const SelectComponent = ({isVirtualized}: {isVirtualized: boolean}) => (
   <Select
     disallowEmptySelection
     className="max-w-xs"
     color="secondary"
     defaultSelectedKeys={["jinx"]}
     isVirtualized={isVirtualized}
     label={`Avatar Decoration ${isVirtualized ? "(Virtualized)" : "(Non-virtualized)"}`}
-    selectedKeys={["jinx"]}
+    aria-label="Select an avatar decoration"
+    onSelectionChange={(key) => {
+      if (!AVATAR_DECORATIONS[Object.keys(AVATAR_DECORATIONS)[0]].includes(key as string)) {
+        console.warn('Invalid selection:', key);
+      }
+    }}
     selectionMode="single"
     variant="bordered"
   >

Additionally, consider adding a performance optimization:

+const memoizedSections = React.useMemo(
+  () => Object.keys(AVATAR_DECORATIONS).map((key) => (
+    <SelectSection
+      key={key}
+      classNames={{
+        heading: "uppercase text-secondary",
+      }}
+      title={key}
+    >
+      {AVATAR_DECORATIONS[key].map((item) => (
+        <SelectItem key={item} className="capitalize" color="secondary" variant="bordered">
+          {item.replace(/-/g, " ")}
+        </SelectItem>
+      ))}
+    </SelectSection>
+  )),
+  []
+);

 const SelectComponent = ({isVirtualized}: {isVirtualized: boolean}) => (
   <Select
     // ... other props
   >
-    {Object.keys(AVATAR_DECORATIONS).map((key) => (
-      <SelectSection
-        // ... section implementation
-      >
-        {AVATAR_DECORATIONS[key].map((item) => (
-          // ... item implementation
-        ))}
-      </SelectSection>
-    ))}
+    {memoizedSections}
   </Select>
 );

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


export const ValidationBehaviorAria = {
render: ValidationBehaviorAriaTemplate,
args: {
Expand Down
Loading