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(autocomplete): add autoHighlight prop #3829

Open
wants to merge 4 commits into
base: canary
Choose a base branch
from

Conversation

dgz9
Copy link
Contributor

@dgz9 dgz9 commented Oct 1, 2024

Closes # N/A

📝 Description

Introduces a new autoHighlight prop to the Autocomplete component, which automatically highlights the first item in the dropdown list as the user types.

⛳️ Current behavior (updates)

Currently, the Autocomplete component does not automatically highlight any option when the user types. Users need to manually navigate through the list to select an option.

🚀 New behavior

  • A new autoHighlight prop is added to the Autocomplete component.
  • When autoHighlight is set to true, the first item in the dropdown list is automatically highlighted as the user types.
  • Users can immediately press Enter to select the highlighted option without needing to navigate through the list.
  • If isAutoHighlighted is false or not provided, the component behaves as it did previously.

💣 Is this a breaking change (Yes/No):

No

📝 Additional Information

Added SB test and sample to documentation.

Summary by CodeRabbit

  • New Features

    • Introduced an autocomplete component for selecting animals with automatic highlighting for the first suggestion.
    • Added a new property, autoHighlight, to enhance user experience in both autocomplete and listbox components.
  • Documentation

    • Updated documentation for the Autocomplete component to include new features and clarify existing properties.
    • Expanded API section with detailed descriptions of new properties and events related to the autocomplete functionality.
  • Bug Fixes

    • Improved logic for highlighting items in the listbox based on user interactions.

@dgz9 dgz9 requested a review from jrgarciadev as a code owner October 1, 2024 05:33
Copy link

changeset-bot bot commented Oct 1, 2024

🦋 Changeset detected

Latest commit: 2a313e8

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 4 packages
Name Type
@nextui-org/autocomplete Minor
@nextui-org/listbox Minor
@nextui-org/react Patch
@nextui-org/select Patch

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

Copy link

vercel bot commented Oct 1, 2024

@dgz9 is attempting to deploy a commit to the NextUI Inc Team on Vercel.

A member of the Team first needs to authorize it.

Copy link
Contributor

coderabbitai bot commented Oct 1, 2024

Walkthrough

This pull request introduces an autocomplete component that allows users to select animals from a predefined dataset. Key features include the addition of an autoHighlight property, which automatically highlights the first item in the list when the dropdown is opened. The changes span multiple files, including updates to documentation, component props, and internal logic to support this new functionality.

Changes

File Path Change Summary
apps/docs/content/components/autocomplete/auto-highlight.ts Introduced an autocomplete component with an animals dataset and an autoHighlight property.
apps/docs/content/components/autocomplete/index.ts Added import for autoHighlight to autocompleteContent.
apps/docs/content/docs/components/autocomplete.mdx Updated documentation to include autoHighlight and refined existing sections for clarity.
packages/components/autocomplete/tests/autocomplete.test.tsx Added tests for autoHighlight functionality in the Autocomplete component.
packages/components/autocomplete/src/use-autocomplete.ts Added autoHighlight property to Props interface and updated logic to handle automatic highlighting.
packages/components/autocomplete/stories/autocomplete.stories.tsx Introduced autoHighlight property in argTypes, added AutoHighlightTemplate and WithAutoHighlight export.
packages/components/listbox/src/listbox.tsx Added autoHighlight property to Listbox component for conditional highlighting.
packages/components/listbox/src/use-listbox.ts Added autoHighlight property to Props interface, updated destructuring and return object.
packages/components/listbox/src/use-listbox-item.ts Introduced autoHighlighted property to control item highlighting in the listbox.

Possibly related PRs

Suggested labels

👀 Status: In Review

Suggested reviewers

  • wingkwong
  • jrgarciadev

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?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

🧹 Outside diff range and nitpick comments (10)
apps/docs/content/components/autocomplete/auto-highlight.ts (1)

29-44: Refactor App component and approve isAutoHighlight usage

The App component is currently defined as a string, which is not a standard practice and could lead to issues:

  1. It prevents proper syntax highlighting and code analysis tools from working correctly.
  2. It makes the code harder to maintain and update.

However, the usage of the new isAutoHighlight prop is correct and aligns with the PR objectives.

Consider refactoring the App component 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 isAutoHighlight prop is correct and implements the new feature as intended.

packages/components/listbox/src/listbox.tsx (2)

56-56: LGTM: Implementation of isAutoHighlighted in ListboxItem

The isAutoHighlighted prop is correctly implemented in the ListboxItem component. The condition isAutoHighlight && state.selectionManager.focusedKey === item.key ensures 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 changes

The implementation of the isAutoHighlight feature 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:

  1. Ensure that the UseListboxProps interface (likely in a separate file) includes the isAutoHighlight prop with appropriate JSDoc comments.
  2. Update the component's documentation to explain the new isAutoHighlight prop, its default value, and its effect on the Listbox behavior.
  3. 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 readability

The isHighlighted logic has been correctly updated to include the new isAutoHighlighted prop. 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 isAutoHighlighted

The isAutoHighlighted prop has been successfully implemented in the use-listbox-item.ts file. 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 isAutoHighlighted prop.
  • Stories:

    • Create or update storybook stories to demonstrate the usage and behavior of the isAutoHighlighted prop.
  • Documentation:

    • Document the isAutoHighlighted prop 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 implemented isAutoHighlighted functionality

The changes in this file successfully implement the new isAutoHighlighted prop for the Autocomplete component. The implementation:

  1. Adds the new prop to the Props interface.
  2. Sets a default value of false when destructuring props.
  3. Incorporates the new prop into the isHighlighted logic.

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 isAutoHighlighted prop, please run the updated verification scripts below:

This revised script uses -g to specify file patterns, ensuring that rg correctly identifies and searches through .test.ts, .test.tsx, .stories.ts, .stories.tsx, .md, and .mdx files. 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 isAutoHighlight prop 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 isAutoHighlight property 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 useEffect hook for the isAutoHighlight feature 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.size to 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: New AutoHighlightTemplate added.

The AutoHighlightTemplate has been implemented correctly to demonstrate the isAutoHighlight feature. It uses the isAutoHighlight prop and sets it to true, showcasing the new functionality.

A minor suggestion for improvement:

Consider adding a comment explaining the purpose of the isAutoHighlight prop 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 consistency

The 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 the isAutoHighlight prop...

(AUTO_HYPHEN)

📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between a4ab020 and 6bc348e.

📒 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 the isAutoHighlight prop...

(AUTO_HYPHEN)

🔇 Additional comments (17)
apps/docs/content/components/autocomplete/auto-highlight.ts (1)

46-53: Clarify the purpose of the export structure

The current export structure is unusual:

  1. It uses file paths as keys in the react object.
  2. 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 of isAutoHighlight prop

The isAutoHighlight prop has been correctly added to the destructured props from the useListbox hook. 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 correctly

The isAutoHighlighted prop has been added to the Props interface as an optional boolean. This addition aligns with the PR objectives and follows the existing naming conventions.


50-50: LGTM: Default value set appropriately

The isAutoHighlighted prop is correctly destructured with a default value of false. 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 isAutoHighlight prop is correctly added to the useListbox function 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 new isAutoHighlight prop!

The changes in this file successfully implement the isAutoHighlight prop as described in the PR objectives. The prop is well-documented, correctly typed, and properly integrated into the useListbox hook. These changes provide a solid foundation for the new auto-highlight feature in the Autocomplete component.

A few suggestions to consider:

  1. Enhance the prop documentation for better clarity.
  2. 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 isAutoHighlight prop is correctly added to the return object of the useListbox function, 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 isAutoHighlight prop is correctly added to the destructured props with the appropriate default value of false, matching the interface definition.


440-440: LGTM! Correct propagation of the isAutoHighlight prop.

The isAutoHighlight prop is correctly added to the getListBoxProps function'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 the isAutoHighlight prop.

The isAutoHighlight prop is correctly added to the return object of the useAutocomplete hook. This allows components consuming this hook to access the isAutoHighlight value, 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 isAutoHighlight feature 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 isAutoHighlight prop enabled and disabled.

✅ Verification successful

Verified the impact on component behavior and performance.

No performance issues related to the isAutoHighlight feature were found. Additionally, the TODO regarding disableClearable is 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 ts

Length of output: 1247

packages/components/autocomplete/stories/autocomplete.stories.tsx (2)

68-72: LGTM: New isAutoHighlight property added to argTypes.

The isAutoHighlight property has been correctly added to the argTypes object with a boolean control type. This allows users to toggle the auto-highlight feature in the Storybook interface.


1085-1090: LGTM: New WithAutoHighlight export added.

The WithAutoHighlight export has been correctly added to showcase the new AutoHighlightTemplate in the storybook. This will allow users to interact with and test the new isAutoHighlight feature.

apps/docs/content/docs/components/autocomplete.mdx (4)

326-329: New feature documentation looks good

The new "Auto Highlight" section effectively introduces the isAutoHighlight property 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 for isReadOnly property

The description for the isReadOnly property 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 feature

The isAutoHighlight property 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 updates

The Autocomplete component documentation has been successfully updated with the following key changes:

  1. Introduction of the new "Auto Highlight" feature, complete with an explanation and code demo.
  2. Addition of the isAutoHighlight property to the API documentation.
  3. Improved clarity for the isReadOnly property 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";
Copy link
Contributor

Choose a reason for hiding this comment

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

💡 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

@wingkwong wingkwong changed the title feat(autocomplete): add isAutoHighlight prop to automatically highlig… feat(autocomplete): add isAutoHighlight prop Oct 1, 2024
@wingkwong wingkwong self-requested a review October 1, 2024 08:20
@wingkwong wingkwong self-assigned this Oct 1, 2024
Copy link
Member

@wingkwong wingkwong left a comment

Choose a reason for hiding this comment

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

  1. the main logic would be taken in feat(autocomplete): automatically focus first non-disabled item #2186. Please sync the latest canary.
  2. please add tests for that
  3. please add changeset

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

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 isAutoHighlight prop 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

📥 Commits

Files that changed from the base of the PR and between 6a260a3 and 094efcb.

📒 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 isAutoHighlight prop 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.

@dgz9
Copy link
Contributor Author

dgz9 commented Oct 1, 2024

  1. Removed my logic and only left the prop to enabled/disable the highlight feature in feat(autocomplete): automatically focus first non-disabled item #2186
  2. Added tests for the new prop.
  3. Added changeset.

Copy link
Member

@wingkwong wingkwong left a comment

Choose a reason for hiding this comment

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

please rename the prop to autoHighlight.

@dgz9
Copy link
Contributor Author

dgz9 commented Oct 6, 2024

Prop renamed to autoHiglight

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

🧹 Outside diff range and nitpick comments (1)
apps/docs/content/docs/components/autocomplete.mdx (1)

87-88: Improved clarity for Read Only behavior

The updated description provides a clearer explanation of the Read Only state behavior. This change enhances user understanding of how the Autocomplete component functions when isReadOnly is set to true.

Consider adding a brief note about why a user might want to use this feature, to provide more context. For example:

 If you pass the `isReadOnly` property to the Autocomplete, the Listbox will open to display
-all available options, but users won't be able to select any of the listed options.
+all available options, but users won't be able to select any of the listed options. This can be useful for displaying a list of options without allowing interaction.
📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 094efcb and 2a313e8.

📒 Files selected for processing (10)
  • .changeset/angry-pillows-accept.md (1 hunks)
  • 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/tests/autocomplete.test.tsx (1 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)
🚧 Files skipped from review as they are similar to previous changes (9)
  • .changeset/angry-pillows-accept.md
  • apps/docs/content/components/autocomplete/auto-highlight.ts
  • apps/docs/content/components/autocomplete/index.ts
  • packages/components/autocomplete/tests/autocomplete.test.tsx
  • packages/components/autocomplete/src/use-autocomplete.ts
  • packages/components/autocomplete/stories/autocomplete.stories.tsx
  • packages/components/listbox/src/listbox.tsx
  • packages/components/listbox/src/use-listbox-item.ts
  • packages/components/listbox/src/use-listbox.ts
🧰 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 the autoHighlight proper...

(AUTO_HYPHEN)


[uncategorized] ~327-~327: “to the” seems less likely than “to”.
Context: ...f you pass the autoHighlight property to the Autocomplete, the Listbox will show the...

(AI_HYDRA_LEO_CP_TO_THE_TO)

🔇 Additional comments (1)
apps/docs/content/docs/components/autocomplete.mdx (1)

Line range hint 1-1: Summary of Autocomplete documentation changes

The updates to the Autocomplete component documentation introduce a new Auto-Highlight feature and improve the clarity of the Read Only behavior. These changes enhance the overall quality of the documentation. However, there are a few key points that need to be addressed:

  1. Ensure consistency in the naming and hyphenation of the new Auto-Highlight feature throughout the document.
  2. Verify the correct property name for the Auto-Highlight feature (isAutoHighlight or autoHighlight) and use it consistently.
  3. Expand the description of the Auto-Highlight feature to provide more context and use cases.
  4. Update the API section to include the new Auto-Highlight property with its description and default value.

Addressing these points will significantly improve the clarity and completeness of the Autocomplete component documentation.

To ensure all instances of the new feature are consistently named and documented, you can run the following command:

This will help identify any inconsistencies in naming or missed occurrences of the new feature throughout the file.

✅ Verification successful

All instances of the autoHighlight property in apps/docs/content/docs/components/autocomplete.mdx are consistently named using camelCase. No variations such as auto-highlight or auto_highlight were found, ensuring naming consistency throughout the documentation.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

rg -i 'auto.?highlight' apps/docs/content/docs/components/autocomplete.mdx

Length of output: 304


Script:

#!/bin/bash
# Search for exact property name `autoHighlight` in the specified file
rg -w 'autoHighlight' apps/docs/content/docs/components/autocomplete.mdx

# Additionally, search for any hyphenated or snake_case variations to ensure consistency
rg -w 'auto-highlight' apps/docs/content/docs/components/autocomplete.mdx
rg -w 'auto_highlight' apps/docs/content/docs/components/autocomplete.mdx

Length of output: 429

@wingkwong wingkwong changed the title feat(autocomplete): add isAutoHighlight prop feat(autocomplete): add autoHighlight prop Oct 6, 2024
@wingkwong wingkwong assigned wingkwong and unassigned dgz9 Oct 6, 2024
@wingkwong wingkwong self-requested a review October 6, 2024 03:00
Copy link

vercel bot commented Oct 14, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
nextui-docs-v2 ✅ Ready (Inspect) Visit Preview 💬 Add feedback Oct 14, 2024 0:13am
nextui-storybook-v2 ✅ Ready (Inspect) Visit Preview 💬 Add feedback Oct 14, 2024 0:13am

Copy link
Member

@wingkwong wingkwong left a comment

Choose a reason for hiding this comment

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

The following case failed. Please add a new test case to cover that as well.

  1. go to autocomplete story
  2. open autocomplete (either by clicking input or by clicking selection button)
  3. the listbox is open, and the first item is highlighted (expected)
  4. close it
  5. open it again, the first item is not highlighted.

@@ -1061,3 +1081,10 @@ export const FullyControlled = {
...defaultProps,
},
};

export const WithAutoHighlight = {
render: AutoHighlightTemplate,
Copy link
Member

Choose a reason for hiding this comment

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

there's no need to create a new template for that. You can simply use Template one and just pass autoHighlight to args below.

@@ -92,6 +92,11 @@ interface Props<T> extends Omit<HTMLNextUIProps<"ul">, "children"> {
* The menu items classNames.
*/
itemClasses?: ListboxItemProps["classNames"];
/**
* Whether to automatically highlight the first item in the list.
Copy link
Member

Choose a reason for hiding this comment

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

first non-disabled item

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants