-
Notifications
You must be signed in to change notification settings - Fork 96
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
Add option that removes the See All Results link in the search results #491
base: master
Are you sure you want to change the base?
Add option that removes the See All Results link in the search results #491
Conversation
WalkthroughThe pull request introduces a new configuration option Changes
✨ Finishing Touches
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
CodeRabbit Configuration File (
|
✅ Deploy Preview for easyops-cn-docusaurus-search-local ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
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: 0
🧹 Nitpick comments (1)
docusaurus-search-local/src/client/theme/SearchBar/SearchBar.tsx (1)
284-286
: Simplify the condition for clarityInstead of explicitly checking
=== false
, consider using a simpler boolean check:- if(removeSeeAllResults === false){ + if (!removeSeeAllResults) { div.appendChild(a); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
docusaurus-search-local/src/client/theme/SearchBar/SearchBar.tsx
(2 hunks)docusaurus-search-local/src/declarations.ts
(1 hunks)docusaurus-search-local/src/index.ts
(1 hunks)docusaurus-search-local/src/server/utils/generate.ts
(2 hunks)docusaurus-search-local/src/server/utils/validateOptions.ts
(1 hunks)
🔇 Additional comments (5)
docusaurus-search-local/src/declarations.ts (1)
16-16
: Add new boolean property for search results removal.Declaring
removeSeeAllResults
as a boolean here is consistent with the plugin's default. Ensure callers handle it properly even when no explicit user configuration is provided.docusaurus-search-local/src/server/utils/validateOptions.ts (1)
35-35
: New boolean property validated with a default value.Adding
removeSeeAllResults
here with a default offalse
aligns with the PR objective, and the approach is consistent with the rest of the schema.docusaurus-search-local/src/server/utils/generate.ts (1)
Line range hint
23-38
: Consistent export of the new property in generate function.Destructuring
removeSeeAllResults
and exporting it matches the existing pattern. No issues with JSON stringification or naming consistency.docusaurus-search-local/src/index.ts (1)
202-208
: Optional plugin option documented correctly.The addition of
removeSeeAllResults?: boolean
with a clear doc block and default behavior aligns perfectly with the PR’s objective. Good job keeping the documentation thorough.docusaurus-search-local/src/client/theme/SearchBar/SearchBar.tsx (1)
33-33
: New variable import looks goodThis newly introduced variable aligns with the PR goal to conditionally remove the footer link.
@@ -280,7 +281,9 @@ export default function SearchBar({ | |||
const a = searchFooterLinkElement({ query, isEmpty }); | |||
const div = document.createElement("div"); | |||
div.className = styles.hitFooter; | |||
div.appendChild(a); | |||
if(removeSeeAllResults === false){ | |||
div.appendChild(a); |
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.
For me, the main issue currently is when I enter some words in the search box, then press the tab key, it jumped to one of the search target page instead of switching between result items. This won't fix it, will it?
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.
Yeah, this won't fix the tab issue. That functionality is in the @easyops-cn/autocomplete.js library, if I'm not mistaken.
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.
Then I'm not sure what this PR fixes
Description
This PR adds an option, defaulted to false, to remove the
See All Results
link in the search results. This is meant as a workaround for the accessibility bug which prevents keyboard navigation to this link.Related: #490
Summary by CodeRabbit
New Features
removeSeeAllResults
boolean settingConfiguration
removeSeeAllResults
option defaults tofalse