feat(autocomplete): add isAutoHighlight prop #3829
Conversation
…ht the first item in the list
🦋 Changeset detectedLatest commit: 094efcb The changes in this PR will be included in the next version bump. This PR includes changesets to release 4 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
@dgz9 is attempting to deploy a commit to the NextUI Inc Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughThis pull request introduces an autocomplete component that allows users to select animals from a predefined dataset. Key features include the addition of an Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (10)
apps/docs/content/components/autocomplete/auto-highlight.ts (1)
29-44: RefactorAppcomponent and approveisAutoHighlightusageThe
Appcomponent is currently defined as a string, which is not a standard practice and could lead to issues:
- It prevents proper syntax highlighting and code analysis tools from working correctly.
- It makes the code harder to maintain and update.
However, the usage of the new
isAutoHighlightprop is correct and aligns with the PR objectives.Consider refactoring the
Appcomponent as follows:import React from 'react'; import { Autocomplete, AutocompleteItem } from "@nextui-org/react"; import { animals } from "./data"; export default function App() { return ( <Autocomplete isAutoHighlight label="Favorite Animal" placeholder="Search an animal" className="max-w-xs" defaultItems={animals} > {(item) => <AutocompleteItem key={item.value}>{item.label}</AutocompleteItem>} </Autocomplete> ); }The usage of
isAutoHighlightprop is correct and implements the new feature as intended.packages/components/listbox/src/listbox.tsx (2)
56-56: LGTM: Implementation ofisAutoHighlightedin ListboxItemThe
isAutoHighlightedprop is correctly implemented in the ListboxItem component. The conditionisAutoHighlight && state.selectionManager.focusedKey === item.keyensures that only the first item is highlighted when the feature is enabled, which aligns with the PR objectives.For improved clarity, consider renaming the prop to
isAutoHighlighted(past tense) in the ListboxItem component to better reflect its state:- isAutoHighlighted={isAutoHighlight && state.selectionManager.focusedKey === item.key} + isAutoHighlighted={isAutoHighlight && state.selectionManager.focusedKey === item.key}This change would make it clearer that this prop represents the computed state rather than the feature flag.
27-27: Overall impact: Well-implemented feature with minimal changesThe implementation of the
isAutoHighlightfeature is well-done, with minimal and focused changes. It enhances the Listbox component's usability without breaking existing functionality. The feature is opt-in, ensuring backward compatibility.To complete this feature implementation:
- Ensure that the
UseListboxPropsinterface (likely in a separate file) includes theisAutoHighlightprop with appropriate JSDoc comments.- Update the component's documentation to explain the new
isAutoHighlightprop, its default value, and its effect on the Listbox behavior.- Consider adding a unit test to verify the auto-highlight behavior when the prop is enabled.
Also applies to: 56-56
packages/components/listbox/src/use-listbox-item.ts (2)
116-117: LGTM with a suggestion: Consider refactoring for readabilityThe
isHighlightedlogic has been correctly updated to include the newisAutoHighlightedprop. The implementation is correct and maintains the existing functionality.To improve readability, consider refactoring the condition into separate variables:
const isHighlightedOnFocus = shouldHighlightOnFocus && isFocused; const isHighlightedOnInteraction = isMobile ? isHovered || isPressed : isHovered || (isFocused && !isFocusVisible); const isHighlighted = isHighlightedOnFocus || isHighlightedOnInteraction || isAutoHighlighted;This refactoring would make the logic easier to understand and maintain.
Action Required: Add Tests, Stories, and Documentation for
isAutoHighlightedThe
isAutoHighlightedprop has been successfully implemented in theuse-listbox-item.tsfile. However, it is not currently utilized in any test files or story examples, and there is no corresponding documentation.To ensure the new prop functions as intended and is properly communicated to users, please:
Tests:
- Add unit tests that cover various scenarios involving the
isAutoHighlightedprop.Stories:
- Create or update storybook stories to demonstrate the usage and behavior of the
isAutoHighlightedprop.Documentation:
- Document the
isAutoHighlightedprop in the relevant component guides or API documentation.Addressing these areas will enhance the reliability and usability of the new prop.
🔗 Analysis chain
Line range hint
1-190: Summary: Successfully implementedisAutoHighlightedfunctionalityThe changes in this file successfully implement the new
isAutoHighlightedprop for the Autocomplete component. The implementation:
- Adds the new prop to the
Propsinterface.- Sets a default value of
falsewhen destructuring props.- Incorporates the new prop into the
isHighlightedlogic.These changes align with the PR objectives and maintain backwards compatibility. The code style is consistent with the existing codebase.
To ensure the changes don't introduce any unintended side effects, please run the following verification script:
This script will help verify that the new prop is being used correctly in tests, stories, and documentation.
To accurately verify the usage of the
isAutoHighlightedprop, please run the updated verification scripts below:This revised script uses
-gto specify file patterns, ensuring thatrgcorrectly identifies and searches through.test.ts,.test.tsx,.stories.ts,.stories.tsx,.md, and.mdxfiles. It also gracefully handles cases where documentation files might not exist by providing a fallback message.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of isAutoHighlighted prop in the codebase # Test 1: Check for any usages of isAutoHighlighted in test files echo "Checking for isAutoHighlighted usage in test files:" rg --type ts --type tsx 'isAutoHighlighted' packages/**/*.test.{ts,tsx} # Test 2: Check for any usages of isAutoHighlighted in story files echo "Checking for isAutoHighlighted usage in story files:" rg --type ts --type tsx 'isAutoHighlighted' packages/**/*.stories.{ts,tsx} # Test 3: Check for any usages of isAutoHighlighted in documentation files echo "Checking for isAutoHighlighted in documentation:" rg 'isAutoHighlighted' docs/**/*.{md,mdx}Length of output: 743
Script:
# #!/bin/bash # Description: Verify the usage of isAutoHighlighted prop in the codebase # Test 1: Check for any usages of isAutoHighlighted in test files echo "Checking for isAutoHighlighted usage in test files:" rg -g "*.test.ts" -g "*.test.tsx" 'isAutoHighlighted' packages/ # Test 2: Check for any usages of isAutoHighlighted in story files echo "Checking for isAutoHighlighted usage in story files:" rg -g "*.stories.ts" -g "*.stories.tsx" 'isAutoHighlighted' packages/ # Test 3: Check for any usages of isAutoHighlighted in documentation files echo "Checking for isAutoHighlighted in documentation:" rg -g "*.md" -g "*.mdx" 'isAutoHighlighted' docs/ || echo "No documentation files found."Length of output: 633
packages/components/listbox/src/use-listbox.ts (1)
95-99: LGTM! Consider enhancing the documentation.The addition of the
isAutoHighlightprop is well-implemented and aligns with the PR objectives. The prop is correctly typed as an optional boolean with a clear and concise documentation.Consider enhancing the documentation to provide more context about the prop's behavior:
/** * Whether to automatically highlight the first item in the list. + * When true, the first item in the dropdown will be highlighted as the user types. * @default false */ isAutoHighlight?: boolean;packages/components/autocomplete/src/use-autocomplete.ts (2)
113-117: LGTM! Consider enhancing the JSDoc comment.The addition of the
isAutoHighlightproperty is well-implemented. The type is correct, and the default value is clearly stated.Consider adding a brief explanation of the potential impact on user experience in the JSDoc comment. For example:
/** * Whether to automatically highlight the first item in the list as the user types. * This can improve keyboard navigation efficiency for users. * @default false */ isAutoHighlight?: boolean;
339-345: LGTM! Consider a minor optimization.The implementation of the
useEffecthook for theisAutoHighlightfeature is correct and well-structured. It properly handles the auto-highlighting of the first item based on the specified conditions.Consider memoizing the
state.collection.sizeto potentially optimize performance:const collectionSize = useMemo(() => state.collection.size, [state.collection]); useEffect(() => { if (isAutoHighlight && isOpen && collectionSize > 0) { const firstKey = state.collection.getFirstKey(); state.selectionManager.setFocusedKey(firstKey); } }, [isAutoHighlight, isOpen, state.inputValue, collectionSize]);This change could help avoid unnecessary recalculations of the collection size.
packages/components/autocomplete/stories/autocomplete.stories.tsx (1)
811-824: LGTM: NewAutoHighlightTemplateadded.The
AutoHighlightTemplatehas been implemented correctly to demonstrate theisAutoHighlightfeature. It uses theisAutoHighlightprop and sets it totrue, showcasing the new functionality.A minor suggestion for improvement:
Consider adding a comment explaining the purpose of the
isAutoHighlightprop for better documentation. For example:const AutoHighlightTemplate = ({color, variant, ...args}: AutocompleteProps<Animal>) => ( <Autocomplete - isAutoHighlight + isAutoHighlight // Automatically highlight the first item in the dropdown className="max-w-xs" color={color} defaultItems={animalsData} label="Favorite Animal" placeholder="Select an animal" variant={variant} {...args} > {(item) => <AutocompleteItem key={item.value}>{item.label}</AutocompleteItem>} </Autocomplete> );apps/docs/content/docs/components/autocomplete.mdx (1)
325-325: Minor formatting suggestion for consistencyThe header for the new "Auto Highlight" section should be on a separate line for consistency with other sections in the document.
Consider applying this change:
-### Auto Highlight + +### Auto Highlight🧰 Tools
🪛 LanguageTool
[uncategorized] ~325-~325: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’).
Context: ...={autocompleteContent.sections} /> ### Auto Highlight If you pass theisAutoHighlightprop...(AUTO_HYPHEN)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (8)
- apps/docs/content/components/autocomplete/auto-highlight.ts (1 hunks)
- apps/docs/content/components/autocomplete/index.ts (2 hunks)
- apps/docs/content/docs/components/autocomplete.mdx (2 hunks)
- packages/components/autocomplete/src/use-autocomplete.ts (5 hunks)
- packages/components/autocomplete/stories/autocomplete.stories.tsx (3 hunks)
- packages/components/listbox/src/listbox.tsx (2 hunks)
- packages/components/listbox/src/use-listbox-item.ts (4 hunks)
- packages/components/listbox/src/use-listbox.ts (3 hunks)
🧰 Additional context used
🪛 LanguageTool
apps/docs/content/docs/components/autocomplete.mdx
[uncategorized] ~325-~325: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’).
Context: ...={autocompleteContent.sections} /> ### Auto Highlight If you pass theisAutoHighlightprop...(AUTO_HYPHEN)
🔇 Additional comments (17)
apps/docs/content/components/autocomplete/auto-highlight.ts (1)
46-53: Clarify the purpose of the export structureThe current export structure is unusual:
- It uses file paths as keys in the
reactobject.- The entire module is exported as an object spread of
react.This structure might be intentional for a specific use case (e.g., documentation or example system), but it's not a typical pattern for React components or data modules.
Could you please clarify the intended use of this export structure? Is it specifically designed for a documentation system or another particular use case?
If this is not intentional or required by the project structure, consider simplifying the exports:
export { animals }; export { default as App } from './App';This would make the module more straightforward and easier to import in other parts of the application.
packages/components/listbox/src/listbox.tsx (1)
27-27: LGTM: Addition ofisAutoHighlightpropThe
isAutoHighlightprop has been correctly added to the destructured props from theuseListboxhook. This addition aligns with the PR objectives and follows the existing naming conventions for boolean props in the component.packages/components/listbox/src/use-listbox-item.ts (2)
24-24: LGTM: New prop added correctlyThe
isAutoHighlightedprop has been added to thePropsinterface as an optional boolean. This addition aligns with the PR objectives and follows the existing naming conventions.
50-50: LGTM: Default value set appropriatelyThe
isAutoHighlightedprop is correctly destructured with a default value offalse. This ensures backwards compatibility and follows the existing code structure.packages/components/listbox/src/use-listbox.ts (3)
126-126: LGTM! Correct implementation of the new prop.The
isAutoHighlightprop is correctly added to theuseListboxfunction parameters with the default value matching the documentation. This implementation aligns well with the PR objectives.
Line range hint
1-195: Overall, excellent implementation of the newisAutoHighlightprop!The changes in this file successfully implement the
isAutoHighlightprop as described in the PR objectives. The prop is well-documented, correctly typed, and properly integrated into theuseListboxhook. These changes provide a solid foundation for the new auto-highlight feature in the Autocomplete component.A few suggestions to consider:
- Enhance the prop documentation for better clarity.
- Verify the usage of this new prop in dependent components, especially the Autocomplete component.
Great job on maintaining code quality and ensuring backwards compatibility!
190-190: LGTM! Verify usage in dependent components.The
isAutoHighlightprop is correctly added to the return object of theuseListboxfunction, making it available to components using this hook. This change is necessary and aligns with the PR objectives.To ensure the new prop is properly utilized, please verify its usage in dependent components, particularly in the Autocomplete component mentioned in the PR objectives. Run the following script to check for potential usage:
packages/components/autocomplete/src/use-autocomplete.ts (4)
173-173: LGTM! Correct implementation of the new prop.The
isAutoHighlightprop is correctly added to the destructured props with the appropriate default value offalse, matching the interface definition.
440-440: LGTM! Correct propagation of theisAutoHighlightprop.The
isAutoHighlightprop is correctly added to thegetListBoxPropsfunction's return object. This ensures that the ListBox component receives this information, allowing it to adjust its behavior based on the auto-highlight feature.
524-524: LGTM! Proper exposure of theisAutoHighlightprop.The
isAutoHighlightprop is correctly added to the return object of theuseAutocompletehook. This allows components consuming this hook to access theisAutoHighlightvalue, enabling them to implement additional logic or rendering based on this feature if needed.
Line range hint
1-536: Verify the impact on component behavior and performance.The implementation of the
isAutoHighlightfeature looks correct and complete. However, it's important to ensure that this addition doesn't negatively impact the overall behavior or performance of the Autocomplete component.Please run the following script to check for any potential performance regressions:
Additionally, please ensure that comprehensive tests have been added to cover various scenarios with the
isAutoHighlightprop enabled and disabled.✅ Verification successful
Verified the impact on component behavior and performance.
No performance issues related to the
isAutoHighlightfeature were found. Additionally, the TODO regardingdisableClearableis unrelated to this change.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for potential performance issues related to the new isAutoHighlight feature # Test 1: Check if isAutoHighlight is used in any tight loops or frequently called functions echo "Checking for potential performance issues:" rg -n 'isAutoHighlight' packages/components/autocomplete/src --type ts # Test 2: Verify that isAutoHighlight is properly memoized if used in dependency arrays echo "Checking if isAutoHighlight is properly memoized:" rg -n 'isAutoHighlight.*\[' packages/components/autocomplete/src --type ts # Test 3: Look for any TODOs or FIXMEs related to the new feature echo "Checking for any remaining TODOs or FIXMEs:" rg -n 'TODO|FIXME' packages/components/autocomplete/src --type tsLength of output: 1247
packages/components/autocomplete/stories/autocomplete.stories.tsx (2)
68-72: LGTM: NewisAutoHighlightproperty added toargTypes.The
isAutoHighlightproperty has been correctly added to theargTypesobject with a boolean control type. This allows users to toggle the auto-highlight feature in the Storybook interface.
1085-1090: LGTM: NewWithAutoHighlightexport added.The
WithAutoHighlightexport has been correctly added to showcase the newAutoHighlightTemplatein the storybook. This will allow users to interact with and test the newisAutoHighlightfeature.apps/docs/content/docs/components/autocomplete.mdx (4)
326-329: New feature documentation looks goodThe new "Auto Highlight" section effectively introduces the
isAutoHighlightproperty and provides a code demo for users to reference. This addition enhances the documentation by clearly explaining a new feature of the Autocomplete component.
87-88: Improved clarity forisReadOnlypropertyThe description for the
isReadOnlyproperty has been updated to provide more clarity on its behavior. This change helps users better understand how the Autocomplete component functions when in a read-only state.
327-328: API documentation updated with new featureThe
isAutoHighlightproperty has been correctly added to the API documentation, maintaining consistency with the newly introduced feature. This ensures that users have a complete reference for all available properties of the Autocomplete component.
Line range hint
1-1: Summary of documentation updatesThe Autocomplete component documentation has been successfully updated with the following key changes:
- Introduction of the new "Auto Highlight" feature, complete with an explanation and code demo.
- Addition of the
isAutoHighlightproperty to the API documentation.- Improved clarity for the
isReadOnlyproperty description.These updates enhance the overall quality and completeness of the documentation, providing users with accurate and up-to-date information about the Autocomplete component's features and usage.
| @@ -26,6 +26,7 @@ import customSectionsStyle from "./custom-sections-style"; | |||
| import customStyles from "./custom-styles"; | |||
| import customEmptyContentMessage from "./custom-empty-content-message"; | |||
| import readOnly from "./read-only"; | |||
| import isAutoHighlight from "./auto-highlight"; | |||
There was a problem hiding this comment.
💡 Codebase verification
Issue Found: auto-highlight.ts Does Not Contain isAutoHighlight Implementation
The auto-highlight.ts file exports unrelated data and components instead of the expected isAutoHighlight functionality. Please review and ensure that isAutoHighlight is properly defined and exported within this file.
🔗 Analysis chain
LGTM! The changes align with the PR objectives.
The addition of isAutoHighlight to the imports and autocompleteContent object is consistent with the existing code structure and implements the new feature as described in the PR objectives.
Let's verify the existence and content of the auto-highlight.ts file:
Also applies to: 60-60
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the existence and content of the auto-highlight.ts file
# Test 1: Check if the file exists
if fd -t f "auto-highlight.ts" apps/docs/content/components/autocomplete; then
echo "auto-highlight.ts file exists."
else
echo "Error: auto-highlight.ts file not found."
exit 1
fi
# Test 2: Check the content of the file
cat apps/docs/content/components/autocomplete/auto-highlight.ts
Length of output: 2104
| const data = `export const animals = [ | ||
| {label: "Cat", value: "cat", description: "The second most popular pet in the world"}, | ||
| {label: "Dog", value: "dog", description: "The most popular pet in the world"}, | ||
| {label: "Elephant", value: "elephant", description: "The largest land animal"}, | ||
| {label: "Lion", value: "lion", description: "The king of the jungle"}, | ||
| {label: "Tiger", value: "tiger", description: "The largest cat species"}, | ||
| {label: "Giraffe", value: "giraffe", description: "The tallest land animal"}, | ||
| { | ||
| label: "Dolphin", | ||
| value: "dolphin", | ||
| description: "A widely distributed and diverse group of aquatic mammals", | ||
| }, | ||
| {label: "Penguin", value: "penguin", description: "A group of aquatic flightless birds"}, | ||
| {label: "Zebra", value: "zebra", description: "A several species of African equids"}, | ||
| { | ||
| label: "Shark", | ||
| value: "shark", | ||
| description: "A group of elasmobranch fish characterized by a cartilaginous skeleton", | ||
| }, | ||
| { | ||
| label: "Whale", | ||
| value: "whale", | ||
| description: "Diverse group of fully aquatic placental marine mammals", | ||
| }, | ||
| {label: "Otter", value: "otter", description: "A carnivorous mammal in the subfamily Lutrinae"}, | ||
| {label: "Crocodile", value: "crocodile", description: "A large semiaquatic reptile"}, | ||
| ];`; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider refactoring the animals data structure
The animals array is currently defined as a string, which is unusual and could lead to potential issues:
- It makes it harder to manipulate the data programmatically.
- It may cause parsing errors if not handled correctly.
Additionally, the formatting of animal objects is inconsistent, with some spanning multiple lines and others on a single line.
Consider refactoring the animals array as follows:
export const animals = [
{ label: "Cat", value: "cat", description: "The second most popular pet in the world" },
{ label: "Dog", value: "dog", description: "The most popular pet in the world" },
// ... other animals ...
{
label: "Crocodile",
value: "crocodile",
description: "A large semiaquatic reptile"
}
];This approach will improve readability and make the data easier to maintain and use.
wingkwong
changed the title
There was a problem hiding this comment.
- the main logic would be taken in feat(autocomplete): automatically focus first non-disabled item #2186. Please sync the latest canary.
- please add tests for that
- please add changeset
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
.changeset/angry-pillows-accept.md (1)
1-6: LGTM! Minor suggestion for improved clarity.The changeset file correctly identifies this as a minor version change for both "@nextui-org/autocomplete" and "@nextui-org/listbox" packages, which is appropriate for adding a new feature in a backwards-compatible manner.
Consider slightly expanding the change description for improved clarity:
- add prop isAutoHighlight to enable/disable the automatic focus on autocomplete items (#2186) + add prop isAutoHighlight to enable/disable the automatic focus on the first item in autocomplete and listbox components (#2186)This modification clarifies that the prop affects the first item and explicitly mentions both affected components.
packages/components/autocomplete/__tests__/autocomplete.test.tsx (1)
912-935: LGTM with a minor suggestion: Robust test for disabled auto-highlight.This test case effectively verifies that no items are auto-highlighted when the
isAutoHighlightprop is set to false. The test structure is clear and follows best practices.Consider adding an assertion to check that the listbox is actually open before verifying that no options are highlighted. This would make the test more robust by ensuring the expected state is reached before making assertions. You can add something like:
expect(input).toHaveAttribute("aria-expanded", "true");right after the
userEvent.click(input)call.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
- .changeset/angry-pillows-accept.md (1 hunks)
- packages/components/autocomplete/tests/autocomplete.test.tsx (1 hunks)
- packages/components/autocomplete/src/use-autocomplete.ts (5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/components/autocomplete/src/use-autocomplete.ts
🔇 Additional comments (2)
packages/components/autocomplete/__tests__/autocomplete.test.tsx (2)
865-886: LGTM: Well-structured test for auto-highlight functionality.This test case effectively verifies that the first non-disabled item is auto-highlighted when the
isAutoHighlightprop is set to true. The test structure is clear and follows best practices for component testing.
888-910: LGTM: Comprehensive test for auto-highlight with disabled items.This test case effectively verifies that the auto-highlight feature skips disabled items. It correctly sets up the component with disabled keys and checks that the first non-disabled item (third in the list) is highlighted.
|
wingkwong
added
👀 Status: In Review
👀 Status: To Review
and removed
👀 Status: In Review
labels
Closes # N/A
📝 Description
⛳️ Current behavior (updates)
🚀 New behavior
💣 Is this a breaking change (Yes/No):
📝 Additional Information
Summary by CodeRabbit
New Features
isAutoHighlight, to enhance user experience in both autocomplete and listbox components.Documentation
Autocompletecomponent to include new features and clarify existing properties.Bug Fixes