Create a simplified dataset view#2413
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughA print-friendly view feature was implemented for dataset pages. This includes new and updated LESS styles for print formatting, logic to adjust pagination for print, a print toggle button with JavaScript, and conditional rendering in views for print mode. Query parameter handling and body class assignment were also introduced to support the print mode. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Browser
participant View
participant URLsService
participant DatasetPageAssembly
User->>Browser: Clicks Print Toggle Button
Browser->>View: Toggle print mode (update URL/query param)
View->>URLsService: editUrlQueryParams(add/remove 'print')
View->>DatasetPageAssembly: setDatasetFiles/Samples(pageSize, print params)
DatasetPageAssembly-->>View: Return data with adjusted pageSize
View->>Browser: Render print-friendly layout/styles
Suggested labels
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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
|
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
![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
♻️ Duplicate comments (1)
protected/components/DatasetPageAssembly.php (1)
379-386: Apply the same refactoring here as in setDatasetFiles.This method contains the same duplicated logic. Use the helper method suggested above.
🧹 Nitpick comments (4)
protected/views/shared/_printToggle.php (3)
20-22: Improve button accessibility and semantic structure.The button could benefit from better accessibility attributes and semantic structure for screen readers.
-<button class="btn btn-link print-view-link" aria-label="Switch to print view"> +<button class="btn btn-link print-view-link" + aria-label="Switch to print view" + aria-pressed="false" + role="switch"> <span class="label-text">Print view</span> + <span class="sr-only" aria-live="polite"></span> </button>
29-32: Add aria-pressed state management for accessibility.The toggle button should update its aria-pressed state to properly communicate its current state to screen readers.
const update = (toPrint) => { $('body').toggleClass('print', toPrint); $label.text(toPrint ? 'Web view' : 'Print view'); + $btn.attr('aria-pressed', toPrint.toString()); + $btn.find('.sr-only').text(toPrint ? 'Currently in print view' : 'Currently in web view'); };
37-46: Add error handling for URL manipulation.The URL manipulation could fail in some edge cases. Consider adding error handling to prevent JavaScript errors.
$btn.on('click', () => { + try { isPrint = !isPrint; if (isPrint) { url.searchParams.set('print', 'true'); } else { url.searchParams.delete('print'); } history.replaceState(null, '', url); update(isPrint); + } catch (error) { + console.error('Error updating print view state:', error); + // Fallback: just toggle the class without URL update + isPrint = !isPrint; + update(isPrint); + } });less/layout/print.less (1)
4-4: Excessive use of !important declarationsMultiple
!importantdeclarations throughout the file suggest potential specificity conflicts. Consider restructuring selectors to achieve proper cascade without forcing importance.Also applies to: 11-11, 15-15, 35-35
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
gigadb/app/services/URLsService.php(1 hunks)less/index.less(1 hunks)less/layout/print.less(1 hunks)less/pages/dataset-print.less(1 hunks)less/pages/dataset.less(1 hunks)protected/components/DatasetPageAssembly.php(2 hunks)protected/views/dataset/view.php(12 hunks)protected/views/layouts/main.php(1 hunks)protected/views/shared/_printToggle.php(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*
Instructions used from:
Sources:
⚙️ CodeRabbit Configuration File
**/*.php
Instructions used from:
Sources:
⚙️ CodeRabbit Configuration File
**/*.less
Instructions used from:
Sources:
⚙️ CodeRabbit Configuration File
🔇 Additional comments (12)
protected/views/layouts/main.php (1)
41-41: LGTM - Clean conditional class implementation.The conditional class assignment correctly implements the print view toggle functionality. The string comparison with 'true' is safe and appropriate for this use case.
less/index.less (1)
78-79: LGTM - Proper import placement for print styles.The print-specific stylesheets are correctly imported at the end of the file, ensuring they can override other styles as needed. The naming convention follows the established pattern.
less/layout/print.less (1)
34-36: Verify custom element selector in print.lessThe
hypothesis-sidebarrule in less/layout/print.less hides a custom element that’s injected at runtime by the Hypothes.is embed. Since we don’t see any static<hypothesis-sidebar>in your templates, please:• Confirm that the Hypothesis embed actually mounts a
<hypothesis-sidebar>element in the print DOM (e.g. via browser print preview).
• If the library uses a different wrapper, class or data-attribute, update the selector to target that instead (for example, a.hypothesis-sidebarclass or[data-hypothesis-sidebar]).This will ensure the rule takes effect in all browsers and print contexts.
less/pages/dataset.less (1)
2-192: Well-structured scoping for print vs screen stylesThe approach of wrapping existing styles in
body:not(.print)effectively separates print and screen styles. The responsive design for the print button container is properly implemented with appropriate breakpoints.less/pages/dataset-print.less (3)
109-115: Proper handling of numeric ID selectorsThe escaped selectors
#\33 dmodelsand#\33 dsketchfabcorrectly handle IDs that start with numbers, which is good CSS practice.
24-32: Excellent UX enhancement for print linksThe implementation of appending URLs to links in print view while excluding internal anchors and JavaScript links is a thoughtful user experience improvement.
123-195: Comprehensive table restructuring for printThe table transformation from tabular to linear layout using
data-headerattributes is well-implemented and will provide a much better print experience.protected/views/dataset/view.php (5)
30-32: Print toggle integration looks goodThe print toggle button is properly integrated using a partial view, which maintains good separation of concerns.
332-358: Well-implemented conditional print paginationThe logic for showing/hiding pagination controls in print mode is properly implemented with appropriate URL manipulation and user-friendly messaging.
388-394: Consistent data-header attribute implementationThe systematic addition of
data-headerattributes to table cells provides proper labeling for the print stylesheet transformation.Also applies to: 465-479, 536-540, 604-606
467-467: Verify print-hidden class usageThe
print-hiddenclass is applied to a table cell that appears to have performance-related TODO comments. Ensure this is intentional and not masking an underlying issue.
1-3: Please verifyURLsServicesanitization and error handling
Ensure that any query-parameter edits are properly validated, escaped, and that the service gracefully handles invalid or missing inputs.• Confirm the implementation of
editUrlQueryParams(or equivalent) sanitizes all incoming values.
• Check for appropriate error catching or fallbacks when URL parameters are malformed.
• Add unit tests or input validation logic if missing to prevent XSS or injection risks.
2 participants
Pull request for issue: #152
Next actionable: deploy this feature to a preview branch and share it with management for feedback on the appearance before reviewing and merging, as management might want to propose changes to the UI of the for print view, e.g. more compact content, more presence of gigadb brand, etc
Description
This pull request introduces a printer-friendly version of the dataset view page. The goal is to provide a clean, simplified layout optimized for printing by removing unnecessary UI elements like the header, footer, navigation, and other interactive components.
A "Print view" toggle button has been added to allow users to switch between the standard and print-optimized modes without a page reload. This improves usability and ensures the printed output is clean, readable, and focused on the essential dataset information.
the following screenshots summarize how this feature works from a user perspective
Normal view showing the location of the toggle view button:
Print view, showing a
print=trueurl query param and a simplified print ready viewButton (link) to toggle the full list of results. It is displayed on the condition:
$totalNbFiles > 100 || $totalNbSamples > 100after the full list of results is on display. Controlled by URL query param
full=trueChanges Made
Backend
URLsService.php: Added aeditUrlQueryParams()method to dynamically manage URL query parameters, used for toggling the complete file/sample list in print view.DatasetPageAssembly.php: Updated to detect the print view via URL parameters and adjust pagination accordingly. It now displays up to 100 items by default in print view, with an option to show all results for larger datasets.dataset/view.php: Modified to include a print view toggle, adddata-headerattributes for improved table layouts in print, and conditionally render elements specific to the print view.layouts/main.php: Adjusted to add aprintclass to the<body>tag when the print view is active.Frontend
_printToggle.php: Created a new shared partial that provides the "Print view" / "Web view" toggle button and the associated JavaScript to update the UI and URL.less/:layout/print.lessandpages/dataset-print.lessto define the styles for the printer-friendly layout.index.lessto import the new print styles.pages/dataset.lesstobody:not(.print)to prevent conflicts with the print view.How to Test
?print=true. The page layout should change to a simplified format, with the header, footer, and side navigation hidden.&full=trueto the URL and display all items.?print=trueparameter should be removed from the URL.docker-compose run --rm codecept run --no-redirect -g issue-152 acceptance -vvChanges to tests
Numerous acceptance tests are added that fully cover this feature without being strict about the appearance and layout
docker-compose run --rm codecept run --no-redirect -g issue-152 acceptance -vvSummary by CodeRabbit
New Features
Style
Bug Fixes