Subpage Navigation via Buttons #170
Conversation
|
WalkthroughThe pull request introduces multiple enhancements to the application's user interface and localization capabilities. Key changes include the addition of new translation keys in the Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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: 8
🧹 Outside diff range and nitpick comments (3)
_layouts/cloud.html (1)
11-20: Enhance accessibility with ARIA labels.The dropdown button could benefit from more descriptive ARIA labels to improve accessibility.
<button class="btn btn-outline-primary mt-1 dropdown-toggle" type="button" id="switchViewButton" data-bs-toggle="dropdown" aria-haspopup="true" aria-expanded="false" + aria-label="{{ site.data.translations._layouts['browse.html']['switch-view'][site.lang] | default: 'Switch View' }}" >_sass/_custom.scss (2)
97-97: Consider adding a transition for smoother focus effect.The inset shadow provides a more subtle and modern focus state. To enhance the user experience further, consider adding a transition for the box-shadow property.
.form-control { &:focus { border-width: 1px; border-color: #ffe880; - box-shadow:inset 0 0 0 .15rem #ffe880; + box-shadow:inset 0 0 0 .15rem #ffe880; + transition: box-shadow 0.15s ease-in-out; } }
101-103: Ensure consistent styling with form-control focus state.The new filterTextBox styling maintains visual consistency with the yellow theme. However, to ensure a cohesive user experience, consider matching the border-width with the form-control focus state.
#filterTextBox { - border-color: #ffe880; + border-color: #ffe880; + border-width: 1px; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
_data/config-nav.csvis excluded by!**/*.csv
📒 Files selected for processing (7)
- _data/translations.yml (1 hunks)
- _includes/js/table-js.html (2 hunks)
- _layouts/browse.html (1 hunks)
- _layouts/cloud.html (1 hunks)
- _layouts/list.html (1 hunks)
- _layouts/timeline_edtf.html (1 hunks)
- _sass/_custom.scss (1 hunks)
🔇 Additional comments (12)
_layouts/cloud.html (3)
Line range hint
35-65: LGTM! Clean integration with existing cloud functionality.The new navigation dropdown is well-integrated with the existing cloud generation logic, maintaining clear separation of concerns.
22-30: Verify translation key availability.Ensure that all translation keys used in this template are properly defined in
_data/translations.yml.✅ Verification successful
Translation keys are properly defined and available
The translation keys used in the template are correctly defined in the translations file. Specifically:
browse.html.browseis defined with translations for en/es/debrowse.html.timelineis defined with translations for en/es/debrowse.html.subjectsis defined with translations for en/es/de🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if the translation keys exist in translations.yml rg -A 3 "'browse.html':" _data/translations.ymlLength of output: 55
Script:
#!/bin/bash # Let's check if translations.yml exists and its content fd -t f "translations.yml" _data/ --exec cat {} # Also check for any other translation files that might contain these keys fd -t f ".*translations.*" _data/ --exec cat {}Length of output: 56075
8-34: Verify alignment with PR objectives regarding dropdowns.The PR objectives mention removing dropdown menus, but this implementation adds a new dropdown for view switching. While this might be intentionally different from the navbar dropdowns being removed, we should verify this approach aligns with the intended navigation structure.
✅ Verification successful
The view-switching dropdown is consistent with the site's navigation pattern
The implementation of the view-switching dropdown in
cloud.htmlis actually consistent with the existing pattern used across the site. The PR's objective about removing dropdown menus specifically refers to the main navigation dropdowns incollection-nav.html, while this is a different UI component used consistently for view switching in various layout templates (browse.html, timeline.html, etc.). The view switcher dropdown is a well-established pattern in this codebase that helps users navigate between different visualization modes of the same content.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other dropdown implementations to understand the navigation pattern rg -l "dropdown-toggle|dropdown-menu" --type htmlLength of output: 283
Script:
#!/bin/bash # Let's check the content of collection-nav files to understand if dropdowns are being removed there echo "=== collection-nav.html ===" cat _includes/collection-nav.html echo -e "\n=== collection-nav-sgb.html ===" cat _includes/collection-nav-sgb.html # Also check timeline and browse layouts to see the navigation pattern echo -e "\n=== timeline.html ===" cat _layouts/timeline.html echo -e "\n=== browse.html ===" cat _layouts/browse.htmlLength of output: 15710
_layouts/browse.html (1)
36-36: LGTM: Improved visual spacing.The added margin-top creates better vertical spacing between the search form and the results counter.
_includes/js/table-js.html (3)
30-30: LGTM! Button text change improves clarity.The change from "exportieren" to "herunterladen" makes the action more intuitive for users.
47-52: LGTM! Export button text is more descriptive.The addition of "exportieren" in the button text makes the action more explicit while maintaining the same functionality.
Line range hint
30-58: Verify text consistency across files.The button text changes look good, but let's ensure consistency with translations in other files.
✅ Verification successful
Let me search for any other language variations or translations to ensure completeness.
Button text changes are consistent with existing translations
The changes are consistent with the German language interface of the application. The search results show:
- "Tabelle" and "Sammlungsdaten" terms are already used in translations
- The button texts follow the established German language pattern
- No conflicting or inconsistent translations were found in other files
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for consistency of button labels across files # Expected: Button labels should match in translations and templates # Check for button text in translation files echo "Checking translations..." rg -i "tabelle.*herunterladen|sammlungsdaten.*exportieren|datenarchiv" _data/translations.yml # Check for button text in other template files echo "Checking other template files..." rg -i "tabelle.*herunterladen|sammlungsdaten.*exportieren|datenarchiv" _includes/ _layouts/Length of output: 546
Script:
#!/bin/bash # Search for any other language variations of these buttons # Including potential English translations or other language files # Check all yml files for similar button patterns echo "Checking all yml files..." fd -e yml -x rg -i "table.*download|collection.*export|data.*archive|tabelle|sammlungsdaten|datenarchiv" {} # Check for any language-specific template variations echo "Checking template variations..." fd -e html -x rg -i "table.*download|collection.*export|data.*archive|tabelle|sammlungsdaten|datenarchiv" {}Length of output: 808
_layouts/list.html (2)
27-51: LGTM! Well-structured sorting dropdown implementation.The flexbox layout with Bootstrap classes provides good alignment, and the sorting dropdown is properly implemented with accessibility considerations.
27-76: Verify mobile responsiveness of the new dropdowns.While the implementation uses Bootstrap's responsive classes, please ensure that:
- Dropdowns don't overflow on small screens
- Buttons remain properly aligned on mobile devices
- Text doesn't wrap awkwardly within the buttons
Test the mobile layout with different screen sizes:
✅ Verification successful
Mobile responsiveness appears to be properly implemented
The implementation uses Bootstrap's responsive classes appropriately:
d-flex justify-content-between align-items-centerensures proper alignmentbtn-outline-primaryuses Bootstrap's responsive button sizingdropdown-menuanddropdown-toggleuse Bootstrap's built-in responsive behaviormt-1provides consistent spacing across devicesms-autohandles proper margin spacing responsivelyThe code follows Bootstrap's mobile-first approach and will adapt well to different screen sizes without custom media queries needed.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for any custom mobile-specific styles that might affect these dropdowns rg -l "[@]media.*?max-width" --type cssLength of output: 153
Script:
#!/bin/bash # Let's check for any custom CSS that might affect these dropdowns in our project rg -g "assets/css/*" "dropdown|button|@media" --type css -C 2 # Also check if there are any specific mobile-related styles in the main stylesheet rg -g "assets/css/style.css" "(@media|dropdown|\.btn)" --type css -C 2Length of output: 283
Script:
#!/bin/bash # Let's try finding CSS files in the repository first fd -e css # Then check the content of any custom CSS files rg "dropdown|button|@media" --type cssLength of output: 184858
_sass/_custom.scss (1)
97-103: Verify consistent styling across related UI components.These styling changes appear to be part of broader UI improvements. Let's verify the styling consistency across related components.
✅ Verification successful
Let me analyze the styling changes and gather more information to verify consistency.
Styling is consistently applied across related UI components
The search results show that the
#ffe880color is used consistently throughout the codebase for:
- Box shadows with inset style
- Border colors
- Background colors
- Text/fill colors
The filter textbox styling aligns with this color scheme and the broader UI patterns. The box-shadow and border-color properties match other interactive elements using the same color value.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for any inconsistent color usage or related styling patterns rg -g "*.{scss,css}" --no-filename "box-shadow|border-color" | grep -i "#ffe880" # Look for other filter-related styling rg -g "*.{scss,css}" --no-filename "filter|search"Length of output: 169653
Script:
#!/bin/bash # Search for other instances of #ffe880 color usage rg -g "*.{scss,css}" "#ffe880" # Look for related focus/hover states and box-shadow styles rg -g "*.{scss,css}" "box-shadow.*inset|:focus|:hover" # Check for filter-related styling patterns rg -g "*.{scss,css}" "#filterTextBox|filter-"Length of output: 185215
_layouts/timeline_edtf.html (1)
29-34: LGTM! Clean implementation of the year range navigation.The flexbox layout ensures proper alignment and the year range links provide efficient navigation through the timeline.
_data/translations.yml (1)
601-620: LGTM! New translations align with PR objectives.The added translations support the PR's goal of enhancing navigation through dedicated buttons. The translations are consistent with existing patterns and cover all the new UI elements mentioned in the PR objectives:
- Switch view functionality
- Browse navigation
- Timeline view
- Subjects view
- Eras view
There was a problem hiding this comment.
Hi @mtwente, thanks for your PR. It solves a lot of latent problems. Here's my 5 cents:
- On
/browse.htmlplease move the button group to the left and change the dropdown to a btn-group-toggle (see https://getbootstrap.com/docs/4.0/components/buttons/#checkbox-and-radio-buttons). - On
/data.htmlcan you please add some text explaining the features (i.e. Zenodo Archive etc.)? - Please review the feedback from coderabbitai. Especially the localisation seems to be inconsistent.
Like this? And on the other pages, I keep it as a dropdown and on the right side? 
|
This looks great! Thank you. If possible we should keep the UX the same on timeline and subjects as well. |
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
- _data/translations.yml (1 hunks)
- _includes/js/table-js.html (2 hunks)
- _layouts/browse.html (1 hunks)
- _layouts/cloud.html (1 hunks)
- _layouts/list.html (1 hunks)
- _layouts/timeline_edtf.html (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- _data/translations.yml
- _includes/js/table-js.html
- _layouts/cloud.html
- _layouts/list.html
- _layouts/timeline_edtf.html
🔇 Additional comments (2)
_layouts/browse.html (2)
Line range hint
72-89: LGTM! Loading and results display implementation looks good.The implementation includes:
- Accessible loading indicator with proper ARIA attributes
- Translated loading text
- Clear visual feedback for users
22-36: Verify translations for view switching options.Following up on previous comments about missing translations, let's verify if all required translations are now available.
✅ Verification successful
All translations for view switching options are properly configured
Based on the translations file content, all the required translations for the view switching options in
browse.htmlare properly configured:
- Timeline:
timeline: en: "Timeline" es: "Línea de tiempo" de: "Zeitachse"
- Subjects:
subjects: en: "Subjects" es: "Temas" de: "Themen"
- Eras:
eras: en: "Eras" es: "Épocas" de: "Epochen"All these translations are properly defined under
_layouts.browse.htmlsection in the translations file and are correctly referenced in the template usingsite.data.translations._layouts['browse.html']path.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check translations for view switching options echo "Checking translations in _data/translations.yml..." rg -A 2 "'timeline'|'subjects'|'eras'" _data/translations.yml # Also check for any untranslated strings echo -e "\nChecking for potential missing translations..." for lang in en de fr es; do echo "Checking $lang translations..." rg "\\[$lang\\]" _data/translations.yml doneLength of output: 813
Script:
#!/bin/bash # Let's check if the translations file exists and its content echo "Checking if translations file exists and its content:" if [ -f "_data/translations.yml" ]; then cat "_data/translations.yml" else echo "translations.yml not found in _data directory" # Let's check where translations might be stored fd "translations" --type f fi # Also check the browse.html layout for translation keys echo -e "\nChecking browse.html for translation keys:" cat "_layouts/browse.html"Length of output: 31103
This reverts commit 9d939e094ef654cc9507a7b098f7488581bd4fd5.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
pages/data.md (2)
9-9: Fix whitespace between sentences.
Add spaces between sentences to improve readability.
-Die Stadt.Geschichte.Basel macht projektbezogene Daten in verschiedenen Formaten zugänglich. Verfügbar sind wissenschaftliche Publikationen mit Projektbezug einerseits, sowie die Metadaten zu den auf dieser Plattform bereitgestellten Quellen und historischen Forschungsdaten andererseits.
+Die Stadt.Geschichte.Basel macht projektbezogene Daten in verschiedenen Formaten zugänglich. Verfügbar sind wissenschaftliche Publikationen mit Projektbezug einerseits, sowie die Metadaten zu den auf dieser Plattform bereitgestellten Quellen und historischen Forschungsdaten andererseits.🧰 Tools
🪛 LanguageTool
[whitespace] ~9-~9: Fügen Sie zwischen Sätzen ein Leerzeichen ein.
Context: ...data.html --- # Datenarchiv Die Stadt.Geschichte.Basel macht projektbezogene Daten in ve...
(DE_SENTENCE_WHITESPACE)
[whitespace] ~9-~9: Fügen Sie zwischen Sätzen ein Leerzeichen ein.
Context: ...-- # Datenarchiv Die Stadt.Geschichte.Basel macht projektbezogene Daten in verschie...
(DE_SENTENCE_WHITESPACE)
[typographical] ~9-~9: Zeichen ohne sein Gegenstück: ‚"‘ scheint zu fehlen
Context: ...erungsfreie-metadaten/){:target="_blank"} erstellt und aufbereitet worden. ## P...
(DE_UNPAIRED_QUOTES)
13-13: Consider rephrasing for better style (optional).
The phrase "im Zusammenhang mit" could be replaced with alternatives like "aus" or "zu" for more concise writing.
-Wissenschaftliche Veröffentlichungen, die im Zusammenhang mit dem Forschungsprojekt stehen,
+Wissenschaftliche Veröffentlichungen aus dem Forschungsprojekt🧰 Tools
🪛 LanguageTool
[style] ~13-~13: Der Ausdruck ‚im Zusammenhang mit‘ kommt sehr häufig vor. Möchten Sie ihn durch eine Alternative ersetzen, um Ihren Text individueller zu machen?
Context: ...ssenschaftliche Veröffentlichungen, die im Zusammenhang mit dem Forschungsprojekt stehen, sind [auf Zenodo archiviert](ht...
(IN_ZUSAMMENHANG_MIT)
[uncategorized] ~13-~13: Hier scheint ein Satzzeichen überflüssig zu sein. Überlegen Sie, es zu löschen.
Context: ... mit dem Forschungsprojekt stehen, sind [auf Zenodo archiviert](https://zenodo.or...
(AI_DE_GGEC_UNNECESSARY_PUNCTUATION_OPEN_BRACKET)
[uncategorized] ~13-~13: Das sieht falsch aus. Überlegen Sie, es zu ersetzen.
Context: ...schungsprojekt stehen, sind auf Zenodo archiviert{:target="_blank...
(AI_DE_GGEC_REPLACEMENT_OTHER)
[typographical] ~13-~13: Zeichen ohne sein Gegenstück: ‚"‘ scheint zu fehlen
Context: ...stadt-geschichte-basel){:target="_blank"}. Die im Rahmen von Konferenzen und Wor...
(DE_UNPAIRED_QUOTES)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
_data/config-nav.csvis excluded by!**/*.csv
📒 Files selected for processing (7)
- _includes/js/table-js.html (2 hunks)
- _layouts/browse.html (1 hunks)
- _layouts/cloud.html (1 hunks)
- _layouts/list.html (1 hunks)
- _layouts/timeline_edtf.html (1 hunks)
- _sass/_custom.scss (2 hunks)
- pages/data.md (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- _includes/js/table-js.html
- _layouts/cloud.html
- _layouts/list.html
- _layouts/timeline_edtf.html
- _sass/_custom.scss
🧰 Additional context used
🪛 LanguageTool
pages/data.md
[whitespace] ~9-~9: Fügen Sie zwischen Sätzen ein Leerzeichen ein.
Context: ...data.html --- # Datenarchiv Die Stadt.Geschichte.Basel macht projektbezogene Daten in ve...
(DE_SENTENCE_WHITESPACE)
[whitespace] ~9-~9: Fügen Sie zwischen Sätzen ein Leerzeichen ein.
Context: ...-- # Datenarchiv Die Stadt.Geschichte.Basel macht projektbezogene Daten in verschie...
(DE_SENTENCE_WHITESPACE)
[typographical] ~9-~9: Zeichen ohne sein Gegenstück: ‚"‘ scheint zu fehlen
Context: ...erungsfreie-metadaten/){:target="_blank"} erstellt und aufbereitet worden. ## P...
(DE_UNPAIRED_QUOTES)
[style] ~13-~13: Der Ausdruck ‚im Zusammenhang mit‘ kommt sehr häufig vor. Möchten Sie ihn durch eine Alternative ersetzen, um Ihren Text individueller zu machen?
Context: ...ssenschaftliche Veröffentlichungen, die im Zusammenhang mit dem Forschungsprojekt stehen, sind [auf Zenodo archiviert](ht...
(IN_ZUSAMMENHANG_MIT)
[uncategorized] ~13-~13: Hier scheint ein Satzzeichen überflüssig zu sein. Überlegen Sie, es zu löschen.
Context: ... mit dem Forschungsprojekt stehen, sind [auf Zenodo archiviert](https://zenodo.or...
(AI_DE_GGEC_UNNECESSARY_PUNCTUATION_OPEN_BRACKET)
[uncategorized] ~13-~13: Das sieht falsch aus. Überlegen Sie, es zu ersetzen.
Context: ...schungsprojekt stehen, sind auf Zenodo archiviert{:target="_blank...
(AI_DE_GGEC_REPLACEMENT_OTHER)
[typographical] ~13-~13: Zeichen ohne sein Gegenstück: ‚"‘ scheint zu fehlen
Context: ...stadt-geschichte-basel){:target="_blank"}. Die im Rahmen von Konferenzen und Wor...
(DE_UNPAIRED_QUOTES)
🔇 Additional comments (5)
pages/data.md (3)
2-2: LGTM: Title change appropriately reflects broader content scope.
The change from "Metadaten" to "Forschungsdaten" better represents the expanded content that now includes both publications and metadata.
17-18: LGTM: Clear and user-friendly download instructions.
The instructions for downloading and exporting metadata are well-structured and easy to follow. The separation between filtered and complete collection exports is clearly explained.
7-18: Verify integration with new navigation structure.
The content changes align well with the PR's navigation improvements. Let's verify the integration of the new button-based navigation mentioned in the PR objectives.
✅ Verification successful
Based on the gathered information, I can now generate the final response:
Navigation structure has been properly implemented
The content changes in data.md are properly integrated with the new navigation structure. The navigation configuration shows "Daten" is implemented as a top-level link (/data.html) without dropdown menus, aligning with the PR's objective of simplifying navigation. The page is accessible through standard button-based navigation as evidenced by the implementation in _includes/collection-nav-sgb.html and _includes/collection-nav.html.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify the implementation of related navigation elements
# Check for button implementations in related templates
echo "Checking for button implementations in related templates:"
rg -l "Datenarchiv" "_includes/" "_layouts/"
# Check for removed dropdown references
echo "Checking for any remaining dropdown references:"
rg -l "dropdown.*Datenarchiv" "_includes/" "_layouts/"
Length of output: 323
Script:
#!/bin/bash
# Let's try a broader search to understand the navigation structure
# Check for navigation-related files
echo "Navigation-related files:"
fd -t f "nav|menu|header" "_includes/" "_layouts/"
# Look for any navigation implementation files
echo -e "\nChecking navigation implementation:"
rg -l "navigation|menu" "_includes/" "_layouts/"
# Search for Datenarchiv references in all template files
echo -e "\nBroader search for Datenarchiv in all template files:"
fd . "_includes/" "_layouts/" -t f --exec rg -l "Datenarchiv" {}
# Check if there are any remaining dropdown implementations
echo -e "\nChecking for any dropdown implementations:"
rg -l "dropdown|pulldown" "_includes/" "_layouts/"
Length of output: 1149
Script:
#!/bin/bash
# Let's examine the dropdown implementations and navigation structure more closely
# Check the content of collection navigation files
echo "Content of collection-nav-sgb.html:"
cat "_includes/collection-nav-sgb.html"
echo -e "\nContent of collection-nav.html:"
cat "_includes/collection-nav.html"
# Look for specific dropdown usage related to Datenarchiv
echo -e "\nChecking dropdown usage with Datenarchiv context:"
rg -A 5 -B 5 "dropdown.*Datenarchiv|Datenarchiv.*dropdown" "_includes/" "_layouts/"
# Check for button implementations in list layout
echo -e "\nChecking list layout implementation:"
cat "_layouts/list.html"
Length of output: 11873
Script:
#!/bin/bash
# Let's check the navigation configuration to see how Datenarchiv is integrated
# Check the navigation configuration
echo "Checking navigation configuration:"
cat "_data/config-nav.yml"
# Look for any references to data or archive pages
echo -e "\nChecking for data/archive page references:"
rg -l "data|archiv" "_data/"
# Check for button implementations specific to data pages
echo -e "\nChecking for data-related button implementations:"
rg "btn.*data|data.*btn" "_includes/" "_layouts/"
Length of output: 4109
Script:
#!/bin/bash
# Let's check the CSV navigation configuration and translations
# Check the navigation configuration CSV
echo "Checking navigation configuration CSV:"
cat "_data/config-nav.csv"
# Check translations related to data archive
echo -e "\nChecking translations for data archive:"
cat "_data/translations.yml"
# Check theme configuration
echo -e "\nChecking theme configuration:"
cat "_data/theme.yml"
Length of output: 35798
🧰 Tools
🪛 LanguageTool
[whitespace] ~9-~9: Fügen Sie zwischen Sätzen ein Leerzeichen ein.
Context: ...data.html --- # Datenarchiv Die Stadt.Geschichte.Basel macht projektbezogene Daten in ve...
(DE_SENTENCE_WHITESPACE)
[whitespace] ~9-~9: Fügen Sie zwischen Sätzen ein Leerzeichen ein.
Context: ...-- # Datenarchiv Die Stadt.Geschichte.Basel macht projektbezogene Daten in verschie...
(DE_SENTENCE_WHITESPACE)
[typographical] ~9-~9: Zeichen ohne sein Gegenstück: ‚"‘ scheint zu fehlen
Context: ...erungsfreie-metadaten/){:target="_blank"} erstellt und aufbereitet worden. ## P...
(DE_UNPAIRED_QUOTES)
[style] ~13-~13: Der Ausdruck ‚im Zusammenhang mit‘ kommt sehr häufig vor. Möchten Sie ihn durch eine Alternative ersetzen, um Ihren Text individueller zu machen?
Context: ...ssenschaftliche Veröffentlichungen, die im Zusammenhang mit dem Forschungsprojekt stehen, sind [auf Zenodo archiviert](ht...
(IN_ZUSAMMENHANG_MIT)
[uncategorized] ~13-~13: Hier scheint ein Satzzeichen überflüssig zu sein. Überlegen Sie, es zu löschen.
Context: ... mit dem Forschungsprojekt stehen, sind [auf Zenodo archiviert](https://zenodo.or...
(AI_DE_GGEC_UNNECESSARY_PUNCTUATION_OPEN_BRACKET)
[uncategorized] ~13-~13: Das sieht falsch aus. Überlegen Sie, es zu ersetzen.
Context: ...schungsprojekt stehen, sind auf Zenodo archiviert{:target="_blank...
(AI_DE_GGEC_REPLACEMENT_OTHER)
[typographical] ~13-~13: Zeichen ohne sein Gegenstück: ‚"‘ scheint zu fehlen
Context: ...stadt-geschichte-basel){:target="_blank"}. Die im Rahmen von Konferenzen und Wor...
(DE_UNPAIRED_QUOTES)
_layouts/browse.html (2)
8-40: Well-structured navigation implementation!
The new button group implementation is clean and follows best practices:
- Proper responsive classes with flex-wrap
- Consistent button styling
- Localized button text
- Clear navigation structure
73-74: LGTM!
The loading indicator and results container implementation is clean and accessible.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
_layouts/browse.html (1)
51-58: Consider adding a search icon to improve UX.The search button could benefit from a visual indicator. Consider adding a search icon alongside the text.
<button class="btn btn-secondary" type="submit" title="{{ site.data.translations['_layouts']['browse.html']['filter-items'][site.lang] | default: 'Filter items' }}" id="filterButton" > + <i class="fas fa-search me-1" aria-hidden="true"></i> {{ site.data.translations._layouts['browse.html'].search[site.lang] | default: 'Search' }} </button>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
_layouts/browse.html(1 hunks)
🔇 Additional comments (3)
_layouts/browse.html (3)
11-16: LGTM! Button group structure follows best practices.
The button group implementation uses proper semantic HTML with appropriate ARIA roles and Bootstrap classes for layout and responsiveness.
73-74: LGTM! Loading indicator implementation is accessible.
The loading spinner implementation follows accessibility best practices with proper ARIA attributes and visually-hidden text.
Also applies to: 76-84
22-22: Verify all required translations exist.
Let's ensure all translation keys used in this file are properly defined in the translations file.
Also applies to: 29-29, 36-36, 48-48, 49-49, 54-54, 57-57, 65-65
✅ Verification successful
The previous script didn't capture all translation keys correctly. Let's modify our approach to better detect the translation patterns:
We've identified the translation keys used in browse.html, but we need to verify their existence in translations.yml. Let's try a different approach:
Let's try one final approach to check if the translations file exists and verify its content:
All required translations for browse.html are present and properly defined
After examining the translations.yml file, I can confirm that all the translation keys used in browse.html are properly defined under _layouts/browse.html: section, including:
- timeline
- eras
- loading
- reset
- search
- subjects
Each key has translations for multiple languages (en, es, de) and is properly structured within the YAML file.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if all required translations exist in translations.yml
# Expected: All translation keys should be present for all supported languages
echo "Checking for required translation keys..."
rg --no-filename "browse\.html'\.(\w+)\[" _layouts/browse.html | sort -u | while read -r match; do
key=$(echo "$match" | sed -E "s/.*browse\.html'\.(\w+)\[.*/\1/")
echo "Checking key: $key"
rg -A 1 "browse\.html:(\s+)?$key:" _data/translations.yml
done
Length of output: 186
Script:
#!/bin/bash
# Get all translation keys used in browse.html
echo "Translation keys used in browse.html:"
rg "site\.data\.translations\._layouts\['browse\.html'\]\.([\w.]+)" -o --no-filename _layouts/browse.html | sed -E "s/.*'browse\.html'\]\.([^[]+).*/\1/" | sort -u
echo -e "\nChecking translations.yml content for browse.html:"
cat _data/translations.yml | grep -A 5 "_layouts/browse.html:"
Length of output: 485
Script:
#!/bin/bash
# Check each translation key individually
echo "Checking individual translation keys in translations.yml:"
for key in "timeline" "eras" "loading" "reset" "search" "subjects"; do
echo -e "\nChecking key: $key"
rg "_layouts/browse\.html:(\s+)?$key:" _data/translations.yml -A 2
done
# Also check the overall structure
echo -e "\nChecking browse.html section in translations.yml:"
rg "_layouts/browse\.html:" _data/translations.yml -A 10
Length of output: 1486
Script:
#!/bin/bash
# First check if the translations file exists
echo "Checking if translations file exists:"
ls -l _data/translations.yml
echo -e "\nChecking content of translations file:"
cat _data/translations.yml
echo -e "\nChecking directory structure:"
ls -R _data/
Length of output: 28442
There was a problem hiding this comment.
Thanks again for the changes. Here are some minor remarks:
- The buttons are sized differently on
browsevscloud,listandtimeline_edtf - The button group should have the same order on all pages
- The button group should have 4 elements, i.e. links to
browse,cloud,listandtimeline_edtf, on all pages - The button group should indicate which button is active (aria and visually)
Thanks again. I really like it!
I made the buttons bigger on I could change the search bar from large to small.
They do have the same order (Browse/Timeline/List/Eras), it's just that the active page/view is not shown as button (heading only). If I put all four buttons next to each other on all pages, it takes away quite a lot of space (especially on I could try moving the four buttons on their own row, stretch them across the whole page and have the search box below as-is, but that just creates oversized buttons.
The dropdown menu (in its current status) is not that different from scrolling for ages, is it? Especially if we have more content for the stone age etc. Then it does not help to increase the dropdown-menu to show centuries only, it will still be a long menu. |
|
Here are a few visual modifications: Medium-sized buttons, to the left. Large-sized buttons, to the left. Medium-sized buttons, stretched on top. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
_layouts/browse.html (2)
52-58: Enhance search input accessibility.While the input has basic ARIA support, consider adding
aria-controls="browseItems"to better indicate what the search affects.<input type="text" class="form-control flex-nowrap" id="filterTextBox" + aria-controls="browseItems" placeholder="{{ site.data.translations['_layouts']['browse.html']['filter'][site.lang] | default: 'Filter' }} ..." aria-label="{{ site.data.translations['_layouts']['browse.html']['search'][site.lang] | default: 'Search' }}" >
82-88: Enhance loading state feedback.Consider adding ARIA live regions to announce loading state changes to screen readers.
- <div id="loadingIcon" class="text-center"> + <div id="loadingIcon" class="text-center" aria-live="polite"> <div class="spinner-border text-dark" role="status"> <span class="visually-hidden"> {{- site.data.translations._layouts['browse.html'].loading[site.lang] | default: 'Loading' }}...</span > </div> </div>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (9)
_data/translations.yml(1 hunks)_layouts/browse.html(1 hunks)_layouts/cloud.html(1 hunks)_layouts/list.html(1 hunks)_layouts/timeline_edtf.html(1 hunks)_sass/_custom.scss(3 hunks)pages/eras.md(1 hunks)pages/subjects.md(1 hunks)pages/timeline.md(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- pages/eras.md
- pages/subjects.md
- pages/timeline.md
🚧 Files skipped from review as they are similar to previous changes (5)
- _data/translations.yml
- _layouts/cloud.html
- _layouts/list.html
- _layouts/timeline_edtf.html
- _sass/_custom.scss
Yes, I was just trying a few different solutions for that! |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
_layouts/browse.html (1)
91-97: Consider enhancing loading state management.While the loading spinner implementation is accessible, consider adding ARIA live regions to announce loading state changes to screen readers.
Add this attribute to improve accessibility:
- <div id="loadingIcon" class="text-center"> + <div id="loadingIcon" class="text-center" aria-live="polite">_sass/_custom.scss (2)
248-252: Consider adding ARIA roles for better accessibility.The layout styling looks good, but consider adding appropriate ARIA roles to enhance accessibility for these interactive elements.
Add these attributes to the respective HTML elements:
<div class="pagination-controls" role="navigation" aria-label="Pagination"> <div class="search-box" role="search">
254-303: Consider optimizing the responsive styles.While the implementation works well, there are opportunities for optimization:
- Consider using CSS custom properties for repeated values like border radius and colors
- The active state styles could be consolidated
Here's how you could optimize the code:
/* At the top of the file */ :root { --border-radius: .25rem; --color-primary: #3a1e3e; --color-accent: #ffe880; } /* Then in your media queries */ #switch-view { .btn { &.active { color: var(--color-accent); &:not(.btn-group-vertical .btn) { border-radius: var(--border-radius) 0 var(--border-radius) 0; } } &:nth-last-child(2) { border-radius: 0 var(--border-radius) var(--border-radius) var(--border-radius); } } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
_data/translations.yml(1 hunks)_includes/js/switch-view-js.html(1 hunks)_layouts/browse.html(1 hunks)_layouts/cloud.html(1 hunks)_layouts/list.html(2 hunks)_layouts/timeline_edtf.html(2 hunks)_sass/_custom.scss(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- _data/translations.yml
- _layouts/cloud.html
- _layouts/list.html
- _layouts/timeline_edtf.html
🔇 Additional comments (6)
_includes/js/switch-view-js.html (1)
1-26: Verify integration with layout files.
Let's ensure this script is properly integrated with all relevant layout files and that the required elements are present.
✅ Verification successful
Integration verified across layout files
The script is properly integrated and consistent across all relevant layout files:
- All 4 layout files (_layouts/browse.html, _layouts/list.html, _layouts/cloud.html, _layouts/timeline_edtf.html) include the switch-view-js.html script
- Each layout file has the required 'switch-view' element with matching structure:
- Consistent ID attribute
- Matching Bootstrap classes (btn-group, btn-group-toggle, d-flex)
- Same data-toggle and role attributes
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify integration of switch-view-js.html and presence of required elements
echo "Checking for switch-view-js.html inclusion in layouts:"
rg -l 'switch-view-js.html' '_layouts/'
echo -e "\nChecking for 'switch-view' element in layout files:"
rg -l 'id="switch-view"' '_layouts/'
echo -e "\nChecking button group structure:"
rg -A 5 'id="switch-view"' '_layouts/'
Length of output: 1823
_layouts/browse.html (2)
59-85: LGTM! Search form implementation is well structured.
The form implementation shows good practices:
- Proper event handling with preventDefault
- Accessible input group with appropriate ARIA labels
- Reset functionality with proper button type
48-54: Clarify the purpose of the toggle button.
The toggle button's purpose is not immediately clear from its label "Switch View". Consider:
- Adding a more descriptive label that indicates what views are being toggled
- Including a tooltip to explain the functionality
Let's check how the toggle functionality is implemented:
_sass/_custom.scss (3)
93-98: LGTM! Improved form control focus state.
The inset shadow provides better visual feedback while maintaining the theme's color scheme.
102-106: LGTM! Consistent search input styling.
The styling matches the theme's color scheme and maintains consistent border radius.
155-162: LGTM! Clear visual feedback for button states.
The styling provides clear visual distinction for active states while maintaining clean button group appearance.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
_includes/js/table-js.html (2)
30-30: Enhance accessibility of DataTable controls.While the flexbox layout improves visual organization, consider adding ARIA labels to improve screen reader navigation.
-dom: 'B<"d-flex justify-content-between align-items-center flex-nowrap mt-2"<"pagination-controls"l><"search-box"f>>t<"row"<"col-md-6"i><"col-md-6"p>>', +dom: 'B<"d-flex justify-content-between align-items-center flex-nowrap mt-2"<"pagination-controls" role="navigation" aria-label="Results per page"l><"search-box" role="search"f>>t<"row"<"col-md-6"i><"col-md-6" role="navigation" aria-label="Pagination"p>>',
Line range hint
50-55: Add error handling for modal interaction.Consider adding error handling for the modal interaction to gracefully handle cases where the modal fails to load or initialize.
{ text: '{{ more_exports }}', action: function ( e, dt, node, config ) { - $('#dataModal').modal('show'); + try { + const modal = $('#dataModal'); + if (modal.length === 0) { + console.error('Modal element not found'); + return; + } + modal.modal('show').on('shown.bs.modal', function () { + $(this).find('[data-focus-first]').focus(); + }); + } catch (error) { + console.error('Failed to show modal:', error); + } }, className: 'btn btn-outline-primary' }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
_data/translations.yml(2 hunks)_includes/js/table-js.html(2 hunks)
🔇 Additional comments (2)
_includes/js/table-js.html (1)
3-6: Verify translation keys exist in translations.yml.
The code assumes the existence of specific translation keys. Let's verify they are properly defined.
✅ Verification successful
Let me verify the actual content of the translations to ensure they are properly defined with values.
Translation keys are properly defined with appropriate values
All required translation keys (download, filtered_metadata_excel, filtered_metadata_csv, more_exports) are present in _data/translations.yml with translations for English (en), Spanish (es), and German (de) languages. The default values in the code match the English translations, providing proper fallback behavior.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that all required translation keys exist in translations.yml
# Test: Check if all required translation keys are present
echo "Checking for required translation keys..."
rg -A 10 "table-js\.html:" "_data/translations.yml" | rg -q "download:|filtered_metadata_excel:|filtered_metadata_csv:|more_exports:" && echo "✓ All required translation keys found" || echo "✗ Missing translation keys"
Length of output: 311
Script:
#!/bin/bash
# Show the actual content of the translations for table-js.html
rg -A 15 "table-js\.html:" "_data/translations.yml"
Length of output: 544
_data/translations.yml (1)
429-444: LGTM! Download-related translations are complete.
The translations for download-related terms are properly implemented across all three languages (English, Spanish, and German) with consistent formatting.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (4)
_includes/js/switch-view-js.html (2)
18-24: Add TypeScript-style documentation for better maintainability.The debounce implementation would benefit from explicit documentation.
Add JSDoc documentation:
+/** + * Creates a debounced version of a function. + * @template T + * @param {(...args: any[]) => T} fn - The function to debounce + * @param {number} delay - The delay in milliseconds + * @returns {(...args: any[]) => void} - The debounced function + */ const debounce = (fn, delay) => {
39-39: Add initial layout setup and cleanup.The resize handler should run on initial load and clean up on page unload.
Apply this diff:
window.addEventListener('resize', debounce(handleResize, 250)); + +// Initial layout setup +handleResize(); + +// Cleanup +window.addEventListener('unload', () => { + window.removeEventListener('resize', handleResize); +});_layouts/browse.html (2)
10-16: Consider enhancing mobile responsiveness.While the button group layout looks good, it might benefit from additional responsive classes for better mobile display:
- <div class="col-12 col-md-auto text-center text-md-center mb-2 mb-lg-0"> + <div class="col-12 col-md-auto text-center text-md-center mb-3 mb-lg-0"> <div id="switch-view" - class="btn-group btn-group-toggle d-flex justify-content-center" + class="btn-group btn-group-toggle d-flex justify-content-center flex-wrap gap-2" data-toggle="buttons" role="group"
74-81: Consider more intuitive mobile visibility class.The reset button uses
d-none d-sm-flexwhich hides it on extra small screens. Consider using a more intuitive approach:- class="btn btn-outline-secondary text-dark filter d-none d-sm-flex" + class="btn btn-outline-secondary text-dark filter d-none d-sm-inline-block"This change makes the button's visibility behavior more predictable and consistent with typical Bootstrap patterns.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
_includes/js/switch-view-js.html(1 hunks)_includes/js/table-js.html(2 hunks)_layouts/browse.html(1 hunks)_layouts/cloud.html(1 hunks)_layouts/list.html(1 hunks)_layouts/timeline_edtf.html(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- _includes/js/table-js.html
- _layouts/cloud.html
- _layouts/list.html
- _layouts/timeline_edtf.html
🔇 Additional comments (3)
_includes/js/switch-view-js.html (1)
5-15: Previous review comment is still applicable.
The suggested improvements for the toggleButtonView function from the previous review remain relevant.
_layouts/browse.html (2)
91-99: LGTM! Well-implemented loading indicator.
The loading indicator implementation is excellent with proper:
- Semantic HTML structure
- Accessibility attributes
- Translation support
- Visual feedback
47-51: Verify the toggleButtonView implementation.
The toggleButtonView function is called but its implementation is not immediately visible. Let's verify its existence and implementation.
✅ Verification successful
The toggleButtonView implementation is properly implemented and used correctly
The function is well-implemented in _includes/js/switch-view-js.html with proper:
- Null checks for the switch-view element
- Bootstrap class toggling for vertical/horizontal layouts
- Button visibility management
- Responsive behavior with debounced window resize handling
- Integration across multiple layout files
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for toggleButtonView implementation
echo "Searching for toggleButtonView implementation..."
rg -A 5 "function toggleButtonView" _includes/js/
Length of output: 517
Script:
#!/bin/bash
# Let's check the complete implementation and usage context
echo "Checking the complete switch-view-js.html file..."
cat _includes/js/switch-view-js.html
echo -e "\nChecking where this JS file is included..."
rg -l "switch-view-js.html" _layouts/
Length of output: 1529
Pull request
Proposed changes
remove dropdowns from navbar, fixes Enhance Navbar #87
data.htmldata.htmldata.html, could also be added to the footer if necessary).browse.htmltimeline.htmlsubjects.htmleras.htmlminor CSS improvements for
browse.htmlregarding colouring and mobile viewadd "Ansicht umschalten", "Zeitachse", "Themen", "Epochen" as entries to
translations.yml(ignored translations to Spanish...). I am not sure about the wording/translation forbrowse. Now it does not really fit to the other categories (verb vs. noun), but I did not have any good idea for phrasing at the moment.Types of changes
Checklist
Summary by CodeRabbit
Release Notes
New Features
User Interface Enhancements