-
-
Notifications
You must be signed in to change notification settings - Fork 783
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
Fix search history consistency #3680
base: master
Are you sure you want to change the base?
Conversation
Related to asyncapi#3676 Unify the search history between the search bar and search icon. * Introduce a shared search history state in `components/AlgoliaSearch.tsx`. * Update `SearchButton` in `components/AlgoliaSearch.tsx` to use the shared search history state. * Modify `AlgoliaModal` in `components/AlgoliaSearch.tsx` to accept and update the shared search history state. * Update `SearchButton` usage in `components/navigation/NavBar.tsx`, `components/navigation/MobileNavMenu.tsx`, `components/Hero.tsx`, and `components/navigation/DocsMobileMenu.tsx` to pass the shared search history state. --- For more details, open the [Copilot Workspace session](https://copilot-workspace.githubnext.com/asyncapi/website/issues/3676?shareId=XXXX-XXXX-XXXX-XXXX).
WalkthroughThe changes enhance the Algolia search feature by introducing a new search history state in the search context. The modifications update interfaces and function signatures in the AlgoliaSearch component to include a Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant AS as AlgoliaSearch
participant TM as transformItems
participant AM as AlgoliaModal
U->>AS: Initiates search query
AS->>TM: Processes query via transformItems
TM->>AS: Calls setSearchHistory (adds unique query)
AS->>AM: Passes updated searchHistory and setter
AM->>U: Displays search results with history context
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
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
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Welcome to AsyncAPI. Thanks a lot for creating your first pull request. Please check out our contributors guide useful for opening a pull request.
Keep in mind there are also other channels you can use to interact with AsyncAPI community. For more details check out this issue.
❌ Deploy Preview for asyncapi-website failed.Built without sensitive environment variables
|
We require all PRs to follow Conventional Commits specification.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (2)
components/navigation/DocsMobileMenu.tsx (1)
53-66
: LGTM! Great improvements to keyboard shortcut display.The changes enhance the search experience by:
- Using semantic HTML (
kbd
,abbr
) for better accessibility- Implementing a flexible render prop pattern
- Providing clear visual feedback for keyboard shortcuts
Consider extracting the "K" text to a constant or i18n key for better internationalization support:
</abbr>{' '} - K + {SEARCH_SHORTCUT_SUFFIX}components/AlgoliaSearch.tsx (1)
139-143
: Fix code formatting and improve readability.The
transformItems
function needs formatting improvements:
- Add blank line after variable declarations
- Add blank line before return statement
Apply this diff to improve readability:
transformItems={(items) => { const transformedItems = transformItems(items); + setSearchHistory((prevHistory) => [ ...new Set([...prevHistory, ...transformedItems.map((item) => item.query)]) ]); + return transformedItems; }}🧰 Tools
🪛 ESLint
[error] 140-140: Expected blank line after variable declarations.
(newline-after-var)
[error] 141-141: Expected blank line before this statement.
(padding-line-between-statements)
[error] 141-141: Replace
...new·Set([...prevHistory,·...transformedItems.map((item)·=>·item.query)])
with⏎··········...new·Set([...prevHistory,·...transformedItems.map((item)·=>·item.query)])⏎········
(prettier/prettier)
[error] 142-142: Expected blank line before this statement.
(padding-line-between-statements)
🪛 GitHub Actions: PR testing - if Node project
[error] 140-140: Expected blank line after variable declarations.
[error] 141-141: Expected blank line before this statement.
[error] 141-141: Replace
...new·Set([...prevHistory,·...transformedItems.map((item)·=>·item.query)])
with⏎··········...new·Set([...prevHistory,·...transformedItems.map((item)·=>·item.query)])⏎········
.
[error] 142-142: Expected blank line before this statement.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
components/AlgoliaSearch.tsx
(7 hunks)components/Hero.tsx
(2 hunks)components/navigation/DocsMobileMenu.tsx
(1 hunks)
🧰 Additional context used
🪛 ESLint
components/AlgoliaSearch.tsx
[error] 117-117: 'searchHistory' is defined but never used. Allowed unused args must match /^_/u.
(unused-imports/no-unused-vars)
[error] 117-117: 'searchHistory' is defined but never used.
(no-unused-vars)
[error] 140-140: Expected blank line after variable declarations.
(newline-after-var)
[error] 141-141: Expected blank line before this statement.
(padding-line-between-statements)
[error] 141-141: Replace ...new·Set([...prevHistory,·...transformedItems.map((item)·=>·item.query)])
with ⏎··········...new·Set([...prevHistory,·...transformedItems.map((item)·=>·item.query)])⏎········
(prettier/prettier)
[error] 142-142: Expected blank line before this statement.
(padding-line-between-statements)
[error] 304-304: 'searchHistory' is assigned a value but never used.
(unused-imports/no-unused-vars)
[error] 304-304: 'searchHistory' is assigned a value but never used.
(no-unused-vars)
[error] 304-304: 'setSearchHistory' is assigned a value but never used.
(unused-imports/no-unused-vars)
[error] 304-304: 'setSearchHistory' is assigned a value but never used.
(no-unused-vars)
🪛 GitHub Actions: PR testing - if Node project
components/AlgoliaSearch.tsx
[error] 117-117: 'searchHistory' is defined but never used. Allowed unused args must match /^_/u.
[error] 117-117: 'searchHistory' is defined but never used.
[error] 140-140: Expected blank line after variable declarations.
[error] 141-141: Expected blank line before this statement.
[error] 141-141: Replace ...new·Set([...prevHistory,·...transformedItems.map((item)·=>·item.query)])
with ⏎··········...new·Set([...prevHistory,·...transformedItems.map((item)·=>·item.query)])⏎········
.
[error] 142-142: Expected blank line before this statement.
[error] 304-304: 'searchHistory' is assigned a value but never used.
[error] 304-304: 'setSearchHistory' is assigned a value but never used.
⏰ Context from checks skipped due to timeout of 180000ms (1)
- GitHub Check: Lighthouse CI
🔇 Additional comments (5)
components/Hero.tsx (2)
7-7
: LGTM!The import statement has been correctly updated to only import the
SearchButton
component, which aligns with the PR objective of unifying search history functionality.
55-70
: LGTM!The
SearchButton
implementation is clean and correctly structured with:
- Proper styling classes
- Dynamic keyboard shortcut rendering
- Consistent text and icon placement
components/AlgoliaSearch.tsx (3)
21-22
: LGTM!The
ISearchContext
interface has been correctly updated to include search history state management.
42-43
: LGTM!The
AlgoliaModalProps
interface has been properly updated to include search history props.
282-293
: LGTM!The
SearchContext.Provider
andAlgoliaModal
components are correctly configured with the search history state.
@@ -282,7 +301,7 @@ export default function AlgoliaSearch({ children }: { children: React.ReactNode | |||
* @param {ISearchButtonProps} props - The props of the search button | |||
*/ | |||
export function SearchButton({ children, indexName = INDEX_NAME, ...props }: ISearchButtonProps) { | |||
const { onOpen, onInput } = useContext(SearchContext); | |||
const { onOpen, onInput, searchHistory, setSearchHistory } = useContext(SearchContext); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove unused props in SearchButton component.
The searchHistory
and setSearchHistory
props are destructured but never used in the component.
Apply this diff to fix the unused variables:
- const { onOpen, onInput, searchHistory, setSearchHistory } = useContext(SearchContext);
+ const { onOpen, onInput } = useContext(SearchContext);
📝 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.
const { onOpen, onInput, searchHistory, setSearchHistory } = useContext(SearchContext); | |
const { onOpen, onInput } = useContext(SearchContext); |
🧰 Tools
🪛 ESLint
[error] 304-304: 'searchHistory' is assigned a value but never used.
(unused-imports/no-unused-vars)
[error] 304-304: 'searchHistory' is assigned a value but never used.
(no-unused-vars)
[error] 304-304: 'setSearchHistory' is assigned a value but never used.
(unused-imports/no-unused-vars)
[error] 304-304: 'setSearchHistory' is assigned a value but never used.
(no-unused-vars)
🪛 GitHub Actions: PR testing - if Node project
[error] 304-304: 'searchHistory' is assigned a value but never used.
[error] 304-304: 'setSearchHistory' is assigned a value but never used.
Related to #3676
Unify the search history between the search bar and search icon.
components/AlgoliaSearch.tsx
.SearchButton
incomponents/AlgoliaSearch.tsx
to use the shared search history state.AlgoliaModal
incomponents/AlgoliaSearch.tsx
to accept and update the shared search history state.SearchButton
usage incomponents/navigation/NavBar.tsx
,components/navigation/MobileNavMenu.tsx
,components/Hero.tsx
, andcomponents/navigation/DocsMobileMenu.tsx
to pass the shared search history state.Summary by CodeRabbit