improve dropdown button on timeline #180
Conversation
|
WalkthroughThe pull request introduces several modifications across multiple files, primarily focusing on enhancing the timeline page's functionality. Key changes include an extensive update to the year navigation settings in Changes
Assessment against linked issues
Possibly related PRs
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
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: 2
🧹 Outside diff range and nitpick comments (3)
_plugins/sort_edtf.rb (1)
15-15: Consider adding a comment to document the output format.The string format "display_format:original:numeric" is used as a workaround for Liquid's limitations. Adding a comment would help future maintainers understand the format and its purpose.
+ # Returns an array of strings in format "display_format:original:numeric" + # Example: "2024:2024:2024" or "500 v. u. Z.:-0500:-500" sorted_dates.map { |date| "#{date[:display_format]}:#{date[:original]}:#{date[:numeric]}" }_layouts/timeline_edtf.html (2)
55-88: Enhance scrollToYear function with user feedback and performance optimizations.The implementation is solid but could benefit from some improvements:
Consider these enhancements:
function scrollToYear(event, year) { event.preventDefault(); + // Debounce scroll events + if (this.scrollTimeout) { + clearTimeout(this.scrollTimeout); + } // Try to find the exact element with data-numericyear equal to the target year let element = document.querySelector(`[data-numericyear="${year}"]`); // If no exact match, find the nearest year if (!element) { const yearElements = Array.from(document.querySelectorAll('[data-numericyear]')); + if (yearElements.length === 0) { + console.warn('No timeline entries found'); + return; + } // Find the element with the closest data-numericyear value let closestElement = null; let closestDifference = Infinity; - yearElements.forEach((el) => { + // Cache numeric year to avoid repeated parsing + const targetYear = parseInt(year, 10); + const years = yearElements.map(el => ({ + element: el, + year: parseInt(el.getAttribute('data-numericyear'), 10) + })); + + years.forEach(({ element, year }) => { - const elementYear = parseInt(el.getAttribute('data-numericyear'), 10); - const difference = Math.abs(elementYear - year); + const difference = Math.abs(year - targetYear); if (difference < closestDifference) { closestDifference = difference; - closestElement = el; + closestElement = element; } }); element = closestElement; } // Scroll to the found element if it exists if (element) { - element.scrollIntoView({ behavior: 'smooth' }); + this.scrollTimeout = setTimeout(() => { + element.scrollIntoView({ behavior: 'smooth' }); + // Highlight the row briefly to show where we scrolled to + element.classList.add('highlight-row'); + setTimeout(() => element.classList.remove('highlight-row'), 2000); + }, 100); } }Add this CSS to highlight the target row:
.highlight-row { animation: highlight 2s ease-out; } @keyframes highlight { 0% { background-color: rgba(255, 255, 0, 0.5); } 100% { background-color: transparent; } }
Line range hint
42-119: Overall implementation review.The changes effectively improve the year dropdown functionality as intended, with good UX considerations like smooth scrolling and finding the nearest year when an exact match isn't found. The implementation is solid but would benefit from the suggested enhancements for validation and user feedback.
Consider adding these architectural improvements:
- Add error boundary handling for the JavaScript functionality
- Consider implementing virtual scrolling for large datasets
- Add aria attributes for better accessibility
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
_data/theme.yml(1 hunks)_data/translations.yml(1 hunks)_layouts/timeline_edtf.html(2 hunks)_plugins/sort_edtf.rb(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- _data/translations.yml
🧰 Additional context used
📓 Learnings (1)
_plugins/sort_edtf.rb (2)
Learnt from: koilebeit
PR: Stadt-Geschichte-Basel/forschung.stadtgeschichtebasel.ch#171
File: _plugins/sort_edtf.rb:35-35
Timestamp: 2024-10-29T10:29:13.216Z
Learning: In `_plugins/sort_edtf.rb`, converting EDTF dates to exact numerical values is intentional to enable sorting and rendering of the data.
Learnt from: koilebeit
PR: Stadt-Geschichte-Basel/forschung.stadtgeschichtebasel.ch#171
File: _plugins/sort_edtf.rb:31-38
Timestamp: 2024-10-29T10:30:35.563Z
Learning: Due to Liquid's limitations in handling complex structures like objects, concatenated strings are returned instead, such as in the `sort_edtf` method in `_plugins/sort_edtf.rb`.
🔇 Additional comments (3)
_plugins/sort_edtf.rb (1)
15-15: LGTM! Verify downstream compatibility.
The addition of numeric values to the output format is well-implemented and aligns with the PR objectives for enhancing the year dropdown functionality.
Let's verify that the new format is properly handled in the timeline layout:
✅ Verification successful
The new format is properly handled in the timeline layout
The timeline layout (_layouts/timeline_edtf.html) correctly processes the new three-part format:
- Line 71: Uses
data-numericyear="{{ year[2] }}"to store the numeric value - Line 72-73: Uses
year[0]for display andyear[1]for the ID - The numeric year is utilized in the
scrollToYearfunction for precise year navigation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the timeline layout correctly processes the new format
# Expected: Find usage of the numeric year value in the timeline layout
# Test: Search for string splitting operations on sort_edtf output
rg -t html "sort_edtf.*\|.*split.*:" _layouts/
Length of output: 48
Script:
#!/bin/bash
# Let's broaden our search to find any usage of sort_edtf in layouts and includes
rg "sort_edtf" _layouts/ _includes/
Length of output: 115
Script:
#!/bin/bash
# Also search for timeline-related files that might process dates
fd "timeline" --type f --exec rg -l "split|date"
Length of output: 201
Script:
#!/bin/bash
# Check if there are any JavaScript files that might handle the date processing
fd "\.js$" --type f --exec rg -l "split.*:|date"
Length of output: 571
Script:
#!/bin/bash
# Look for Liquid filters or tags that might process these dates
rg "{%.*sort_edtf|{%.*date" _layouts/ _includes/
Length of output: 148
Script:
#!/bin/bash
# Let's examine the timeline_edtf.html layout to see how it processes the sorted dates
cat _layouts/timeline_edtf.html
Length of output: 7049
Script:
#!/bin/bash
# Also check the timeline JavaScript implementation
cat _includes/js/timeline-js.html
Length of output: 786
_layouts/timeline_edtf.html (2)
42-46: Add input validation for year parameter.
While the changes improve the dropdown functionality, the year parameter passed to scrollToYear should be validated to prevent potential issues with invalid inputs.
Consider adding validation:
- <a class="dropdown-item" href="#" onclick="scrollToYear(event, {{ y }})">{{ y }}</a>
+ {% assign numeric_year = y | plus: 0 %}
+ {% if numeric_year == y %}
+ <a class="dropdown-item" href="#" onclick="scrollToYear(event, {{ y }})">{{ y }}</a>
+ {% else %}
+ <span class="dropdown-item text-muted">Invalid year: {{ y }}</span>
+ {% endif %}119-119: 🛠️ Refactor suggestion
Add validation for numeric year attribute.
The data-numericyear attribute relies on year[2] without validation. This could lead to invalid attributes if the year array doesn't contain the expected elements.
Add validation:
- <tr id="y{{ year[1] }}" data-numericyear="{{ year[2] }}">
+ {% assign numeric_year = year[2] | default: year[1] | plus: 0 %}
+ <tr id="y{{ year[1] }}" data-numericyear="{{ numeric_year }}">Let's verify the year array structure:
Pull request
Proposed changes
fixes #176
Types of changes
Checklist
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
These changes collectively improve user interaction with the timeline feature and enhance the overall experience.