redirect search to browspage #178
Conversation
|
WalkthroughThe changes involve modifications to the navigation component of a web page, specifically in four files. The Changes
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
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
_includes/collection-nav-sgb.html(1 hunks)_includes/nav-search.html(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- _includes/collection-nav-sgb.html
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
_data/translations.yml(1 hunks)_includes/nav-search.html(1 hunks)
🔇 Additional comments (2)
_includes/nav-search.html (2)
12-20: Add input constraints for better security.
The search input field should have length constraints to prevent excessive long queries:
[security]
<input
id="site-search"
class="form-control"
type="text"
autocomplete="off"
spellcheck="false"
+ minlength="2"
+ maxlength="100"
placeholder="{{ site.data.translations['_includes']['nav-search.html']['search'][site.lang] | default: 'Search' }}"
aria-label="{{ site.data.translations['_includes']['nav-search.html']['search'][site.lang] | default: 'Search' }}"
>10-32: Consider adding CSRF protection.
The form submission should include CSRF protection to prevent cross-site request forgery attacks:
[security]
<form class="form-inline my-2 my-lg-0" role="search" id="search" onsubmit="site_search(); return false;" aria-label="{{ site.data.translations['_includes']['nav-search.html']['search'][site.lang] | default: 'Search' }}">
+ {% csrf_token %}
<div class="input-group">Note: The exact CSRF implementation might vary depending on your web framework. Please adjust accordingly.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
_data/translations.yml(1 hunks)_includes/nav-search.html(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- _data/translations.yml
🔇 Additional comments (1)
_includes/nav-search.html (1)
10-42: 🛠️ Refactor suggestion
Enhance form validation and user feedback.
The form implementation could benefit from additional validation attributes and visual feedback during search.
<form
class="form-inline my-2 my-lg-0"
role="search"
id="search"
- onsubmit="site_search(); return false;"
+ onsubmit="debouncedSearch(); return false;"
aria-label="{{ site.data.translations['_includes']['nav-search.html']['search'][site.lang] | default: 'Search' }}"
>
<div class="input-group">
<input
id="site-search"
class="form-control"
type="text"
autocomplete="off"
spellcheck="false"
+ maxlength="100"
+ minlength="2"
+ pattern="[^<>]*"
+ title="{{ site.data.translations['_includes']['nav-search.html']['validation'][site.lang] | default: 'Please enter a valid search term' }}"
placeholder="{{ site.data.translations['_includes']['nav-search.html']['search'][site.lang] | default: 'Search' }}"
aria-label="{{ site.data.translations['_includes']['nav-search.html']['search'][site.lang] | default: 'Search' }}"
>
<button
class="btn btn-dark"
type="submit"
+ id="search-submit"
aria-label="{{ site.data.translations['_includes']['nav-search.html']['submit'][site.lang] | default: 'Submit search' }}"
>
- <span id="search-icon">
+ <span id="search-icon" class="d-flex align-items-center">
<svg viewBox="0 0 1024 974" fill="white" height="16px" width="20px" aria-hidden="true" focusable="false">
<path d="M960 832L710.875 582.875C746.438 524.812 768 457.156 768 384 768 171.96900000000005 596 0 384 0 171.969 0 0 171.96900000000005 0 384c0 212 171.969 384 384 384 73.156 0 140.812-21.562 198.875-57L832 960c17.5 17.5 46.5 17.375 64 0l64-64C977.5 878.5 977.5 849.5 960 832zM384 640c-141.375 0-256-114.625-256-256s114.625-256 256-256 256 114.625 256 256S525.375 640 384 640z"/>
</svg>
+ <span class="spinner-border spinner-border-sm ms-1 d-none" role="status" id="search-spinner">
+ <span class="visually-hidden">Searching...</span>
+ </span>
</span>
<span class="visually-hidden">
{{- site.data.translations['_includes']['nav-search.html']['search'][site.lang] | default: 'Search' -}}
</span>
</button>
</div>
</form>
+<style>
+.form-control:invalid {
+ border-color: var(--bs-danger);
+}
+</style>
+<script>
+// Add loading indicator during search
+document.getElementById('search').addEventListener('submit', function() {
+ document.getElementById('search-spinner').classList.remove('d-none');
+ document.getElementById('search-submit').disabled = true;
+ setTimeout(() => {
+ document.getElementById('search-spinner').classList.add('d-none');
+ document.getElementById('search-submit').disabled = false;
+ }, 2000);
+});
+</script>Changes include:
- Input validation with maxlength, minlength, and pattern attributes
- Loading spinner during search
- Visual feedback for invalid input
- Debounced search submission
There was a problem hiding this comment.
@koilebeit Thank you very much for this PR. It's a nice fix. My suggestions in regards to the naming are not critical.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
_includes/nav-search-browse.html (2)
10-16: Enhance form security and accessibility.The form implementation should include additional security measures and accessibility improvements:
<form class="form-inline my-2 my-lg-0" role="search" id="search" onsubmit="site_search(); return false;" + method="get" + action="{{ '/browse.html' | relative_url }}" + aria-live="polite" + aria-atomic="true" aria-label="{{ site.data.translations['_includes']['nav-search-browse.html']['search'][site.lang] | default: 'Search' }}" >
17-26: Add input constraints and search hints.Consider adding input constraints and search functionality hints:
<input id="site-search" class="form-control" type="text" autocomplete="off" spellcheck="false" + minlength="2" + maxlength="100" + pattern="[^<>]*" + title="{{ site.data.translations['_includes']['nav-search-browse.html']['search_hint'][site.lang] | default: 'Enter at least 2 characters' }}" placeholder="{{ site.data.translations['_includes']['nav-search-browse.html']['search'][site.lang] | default: 'Search' }}" aria-label="{{ site.data.translations['_includes']['nav-search-browse.html']['search'][site.lang] | default: 'Search' }}" >
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
_data/translations.yml(1 hunks)_includes/collection-nav-sgb.html(1 hunks)_includes/nav-search-browse.html(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- _data/translations.yml
- _includes/collection-nav-sgb.html
🔇 Additional comments (2)
_includes/nav-search-browse.html (2)
27-42: LGTM! Well-implemented accessible search button.
The button implementation follows accessibility best practices with proper ARIA labels, screen reader text, and decorative SVG handling.
1-42: Verify browse page implementation and translations.
Please ensure:
- The browse page exists and can handle hash parameters for search
- All required translations are defined in
_data/translations.yml
✅ Verification successful
Let me gather more information about the browse page implementation since it wasn't found in the root or includes directory.
Let me search for the browse page implementation and its JavaScript handling since we found the layout file but need to verify the functionality.
Let me check for the browse page JavaScript implementation since we found the layout file.
Based on the verification results, I can now provide a final response since we have confirmed:
- The browse page exists at
_layouts/browse.html - The JavaScript implementation in
_includes/js/browse-js.htmlproperly handles hash parameters for search - The translations are defined in
_data/translations.yml
Implementation verified successfully
The search functionality is properly implemented with:
- Hash-based URL parameters for search terms
- Proper handling of URL hash changes for filtering
- Translation support for UI elements
- Client-side filtering and display of results
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if browse page exists and verify translations
echo "Checking for browse page..."
if [ -f "_includes/browse.html" ] || [ -f "browse.html" ]; then
echo "Browse page found"
else
echo "Warning: browse page not found"
fi
echo "Checking for required translations..."
rg -A 2 "nav-search-browse.html" "_data/translations.yml"
Length of output: 393
Script:
#!/bin/bash
# Search for browse page and its implementation
echo "Searching for browse page in all directories..."
fd browse.html
echo -e "\nSearching for browse-related JavaScript implementations..."
rg -g "*.{html,js}" "browse" -A 5
echo -e "\nChecking for Jekyll page configurations..."
fd "_config.yml|*.md" --exec grep -l "browse"
Length of output: 128124
Script:
#!/bin/bash
# Check for browse page implementation and related JavaScript
echo "Checking for browse page JavaScript implementation..."
fd -e js browse
fd -e html browse
echo -e "\nChecking for browse page content handling..."
rg -g "*.{js,html}" "function.*browse" -A 5
Length of output: 90070
Script:
#!/bin/bash
# Check for browse page JavaScript implementation
echo "Checking for browse.js content..."
cat _includes/js/browse-js.html
echo -e "\nChecking for browse page hash handling..."
rg -g "*.{js,html}" "window.location.hash" -A 3
Length of output: 10523
Pull request
Proposed changes
solves #175
Types of changes
Checklist
Summary by CodeRabbit
New Features
Bug Fixes