add global trigger warning #155
Conversation
|
WalkthroughThe changes in this pull request involve modifications to the trigger warning settings for item pages and the introduction of a new modal component for displaying these warnings. The Changes
Possibly related issues
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: 0
🧹 Outside diff range and nitpick comments (4)
_includes/js/trigger-warning-js.html (3)
3-10: ThehideWarningfunction looks good, with some suggestions for improvement.The function correctly hides the modal and sets a session storage item. However, consider the following enhancements:
- Add error handling in case the modal element is not found.
- Consider using local storage instead of session storage if you want the warning to remain dismissed across sessions.
- Ensure that the Bootstrap library is properly loaded before this script runs.
Here's a suggested improvement:
function hideWarning() { const modal = document.getElementById('triggerWarning'); if (!modal) { console.error('Trigger warning modal not found'); return; } const bootstrapModal = bootstrap.Modal.getInstance(modal) || new bootstrap.Modal(modal); bootstrapModal.hide(); // Use localStorage instead of sessionStorage if you want the dismissal to persist across sessions localStorage.setItem('triggerWarningDismissed', 'true'); }
13-21: The window onload event handler is functional, but could be improved.The event handler correctly checks for previous dismissal and shows the modal if needed. However, consider the following enhancements:
- Add error handling for cases where 'triggerWarning' or 'proceedButton' elements are not found.
- Only create the modal instance if it's going to be shown.
- Consider using
addEventListener('load', ...)instead ofwindow.onloadfor better compatibility and to avoid potential overwrites.Here's a suggested improvement:
window.addEventListener('load', function () { const triggerWarning = document.getElementById('triggerWarning'); const proceedButton = document.getElementById('proceedButton'); if (!triggerWarning || !proceedButton) { console.error('Required elements not found'); return; } if (!sessionStorage.getItem('triggerWarningDismissed')) { const modal = new bootstrap.Modal(triggerWarning); modal.show(); } proceedButton.addEventListener('click', hideWarning); });
1-22: Overall, the trigger warning implementation is good, with room for improvement.The script successfully implements a trigger warning modal using Bootstrap, with functionality to show the warning and remember when it has been dismissed. However, there are a few areas where the code could be enhanced:
- Improve error handling and add defensive coding practices.
- Consider the choice between
sessionStorageandlocalStoragebased on the desired persistence of the dismissal.- Ensure that the code doesn't assume the availability of Bootstrap or certain DOM elements.
Next steps:
- Implement the suggested improvements in the previous comments.
- Add comments to explain the purpose of using session/local storage and any other non-obvious logic.
- Consider adding a configuration option to easily switch between session and local storage.
- Ensure that this script is loaded after Bootstrap in your HTML to avoid potential errors.
- Add unit tests to verify the functionality of the
hideWarningfunction and the onload behavior._data/theme.yml (1)
22-23: Consider the implications of centralizing the trigger warningThe changes to the trigger warning configuration raise several points for consideration:
Centralization: Moving the trigger warning to
index.mdcentralizes the warning, but it may not adequately warn users accessing individual items directly. Consider implementing a system that displays the warning on both the index and item pages.Comprehensive text: The new warning text provides a more thorough explanation, which is commendable. However, its length might impact user experience and readability, especially on mobile devices.
Language: Ensure that the German text aligns with your target audience. If you have a diverse user base, consider offering multilingual options.
Accessibility: The length of the text might affect the layout and design of the index page. Consider ways to present the information that maintain both informativeness and good user experience, such as a collapsible section or a modal dialog.
To address these points, consider the following suggestions:
- Implement a system to show the warning on both index and item pages.
- Create a shorter version of the warning for immediate display, with an option to expand for more details.
- Offer language toggle for multilingual support if needed.
- Use a modal or collapsible section to present the full text without overwhelming the main page layout.
Would you like assistance in implementing any of these suggestions?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (9)
- _data/theme.yml (1 hunks)
- _includes/feature/trigger-warning.html (1 hunks)
- _includes/item/trigger-warning.html (0 hunks)
- _includes/js/trigger-warning-js.html (1 hunks)
- _layouts/browse.html (0 hunks)
- _layouts/item/item-page-base.html (1 hunks)
- _layouts/item/item-page-full-width.html (1 hunks)
- pages/index.md (1 hunks)
- pages/sensible-inhalte.md (0 hunks)
💤 Files with no reviewable changes (3)
- _includes/item/trigger-warning.html
- _layouts/browse.html
- pages/sensible-inhalte.md
🧰 Additional context used
🔇 Additional comments (7)
_includes/feature/trigger-warning.html (1)
1-20: Good implementation of the trigger warning modal with some suggestions for improvement.The modal structure is well-implemented using Bootstrap classes and includes appropriate ARIA attributes for accessibility. The dynamic content loading for the warning text is a good approach for maintainability.
Here are some suggestions for improvement:
- Consider moving inline styles to a separate CSS file for better maintainability.
- Make the button text "Verstanden" translatable for better internationalization support.
- Ensure consistency in language use. The default warning text is in English, while other content is in German.
Here's a suggested refactor for the button to make it translatable:
- <button id="proceedButton" type="button" class="btn btn-primary mt-4" aria-label="Hinweis ausblenden"> - Verstanden - </button> + <button id="proceedButton" type="button" class="btn btn-primary mt-4" aria-label="Hinweis ausblenden"> + {{ site.data.theme['trigger-warning-button-text'] | default: 'Verstanden' }} + </button>This change allows you to define the button text in your
_data/theme.ymlfile, making it easier to translate or modify in the future.To ensure consistency in language use and verify the default warning text, run the following script:
This script will help verify the language consistency and check if a German translation is provided in the theme configuration.
✅ Verification successful
Verification Successful: Language Consistency Confirmed
The default warning text in
_includes/feature/trigger-warning.htmlis in English. However, a German translation is provided in_data/theme.yml, ensuring consistent language use when the German locale is active.No further actions are required.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check the default warning text language and theme configuration # Test 1: Check the default warning text in the file echo "Default warning text in _includes/feature/trigger-warning.html:" rg --type html "default: '.*'" _includes/feature/trigger-warning.html # Test 2: Check if there's a German translation in the theme configuration echo "\nChecking for German translation in _data/theme.yml:" rg "trigger-warning-text:" _data/theme.yml -A 1Length of output: 1241
_layouts/item/item-page-full-width.html (3)
3-3: LGTM: Comment update is consistent with changes.The removal of "trigger-warning" from the comment accurately reflects the changes made to this template. This aligns with the PR objective of implementing a global trigger warning system.
Line range hint
1-54: LGTM: Template structure and functionality remain intact.The overall structure and functionality of the template appear to be well-maintained:
- It correctly extends the 'default' layout.
- Essential components (breadcrumbs, title, citation box, rights box, browse buttons) are present.
- The template handles both single items and compound objects appropriately.
- Conditional logic for displaying different elements based on the item type is preserved.
The removal of the trigger warning inclusion hasn't negatively impacted the core functionality of the template. The changes align with the PR objective of implementing a global trigger warning system while maintaining the existing item page structure.
Line range hint
1-54: Verify removal of trigger warning inclusion.The AI summary indicates that the line
{% include item/trigger-warning.html %}has been removed from this template. This change is consistent with the PR objective of implementing a global trigger warning. However, we should verify that this removal doesn't negatively impact the page's functionality or user experience.To confirm the removal and its impact, please run the following script:
This script will help ensure that:
- The trigger warning include has been completely removed from layout files.
- There are no lingering references to trigger warnings in other layout files.
- The new global trigger warning has been implemented in the appropriate include files.
✅ Verification successful
Trigger warning inclusion successfully removed from layout files.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the removal of trigger warning inclusion and check for any remaining references. # Test 1: Check if the trigger-warning.html include has been completely removed echo "Test 1: Checking for trigger-warning.html include" rg --type html 'include.*trigger-warning.html' _layouts/ # Test 2: Search for any remaining references to trigger warnings in layout files echo "Test 2: Searching for trigger warning references in layouts" rg --type html 'trigger.?warning' _layouts/ # Test 3: Verify the new global trigger warning implementation echo "Test 3: Checking for new global trigger warning implementation" rg --type html 'trigger.?warning' _includes/Length of output: 794
pages/index.md (1)
16-21: LGTM! Improved modular approach for trigger warnings.The changes introduce a more flexible and modular approach to displaying trigger warnings. This is an improvement over the previous static alert, allowing for better customization and potentially enhanced user experience. The separation of HTML and JS components is a good practice for maintainability.
To ensure the new components are properly implemented, please run the following verification:
This script will help verify that the new components are in place and have the expected basic structure. Please review the output to ensure everything is set up correctly.
_layouts/item/item-page-base.html (2)
3-3: LGTM: Updated comment accurately reflects current layout functionality.The modified comment correctly describes the current state of the layout, mentioning the addition of breadcrumbs and title at the top, as well as the retention of the citation box, rights box, and browse buttons at the bottom.
Line range hint
1-70: Verify global trigger warning implementation.The removal of the trigger warning component from this layout aligns with the PR objective of implementing a global trigger warning. However, please ensure that:
- The global trigger warning is implemented effectively to cover all necessary content.
- Users are adequately warned before accessing potentially sensitive content on item pages.
- The new implementation (likely using session or local storage as mentioned in the PR description) is thoroughly tested across different scenarios.
To confirm the proper implementation of the global trigger warning, please run the following script:
This script will help verify that the global trigger warning is properly implemented and that individual trigger warnings have been removed from item pages.
✅ Verification successful
Global trigger warning implementation verified.
The global trigger warning component is present in
_includes/feature/trigger-warning.html, and individual trigger warnings have been successfully removed from item pages. No related JavaScript issues were found.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the implementation of the global trigger warning # Test 1: Check for the presence of the global trigger warning component echo "Checking for global trigger warning component:" rg --type html "trigger-warning" # Test 2: Verify the removal of individual trigger warnings from item pages echo "Verifying removal of individual trigger warnings:" rg --type html "trigger-warning" _layouts/item/ # Test 3: Check for the presence of JavaScript related to trigger warnings echo "Checking for trigger warning related JavaScript:" rg --type js "triggerWarning|warning"Length of output: 87692
|
I know the modal was supposed to cover the whole screen, but what do you think about the way this looks: 
I think it works well with the darkened background. I added 85% opacity to the modal's backdrop, I could also go up to 90%. This way the users still get shown a glimpse of the website that they're on – or to put it differently: That they are in fact on a website that consists of more content than the trigger warning... And we have the logo + headline "Forschungsdatenplattform" already kind of visible in the background. The modal is centered horizontally and vertically on the screen. P.S.: "Materialien auf dieser Datenbank"? Not in dieser Datenbank? |
I like it 🥰 And. Also the the fixed typo |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
_sass/_custom.scss (2)
211-213: LGTM: Appropriate background color for trigger warning modal.The light yellow background (#fff3cd) is a good choice for the trigger warning modal, as it draws attention without being overly alarming. The use of the specific ID ensures that this style only applies to the trigger warning modal.
Consider adding a contrast check to ensure that the text color used in the modal has sufficient contrast with this background color for accessibility purposes.
220-230: LGTM with suggestions: Consistent styling for trigger warning button.The
.btn-triggerwarningclass provides a consistent color scheme with the trigger warning modal, which is good for visual coherence. The focus state with box shadow enhances accessibility.Consider the following enhancements for better user interaction feedback:
- Add a subtle background color change on hover for more noticeable feedback.
- Ensure the contrast ratio between text and background colors meets WCAG 2.1 Level AA standards for accessibility.
Here's a suggested modification:
.btn-triggerwarning { border-color: #3a1e3e; background-color: #3a1e3e; color: #fff3cd; &:hover { background-color: lighten(#3a1e3e, 10%); color: #ffe880; } &:focus { box-shadow: 0 0 0 0.25rem rgba(58, 30, 62, 0.5); } }This change lightens the background color on hover and adjusts the focus box shadow for better visibility.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- _data/theme.yml (1 hunks)
- _includes/feature/trigger-warning.html (1 hunks)
- _sass/_custom.scss (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- _data/theme.yml
- _includes/feature/trigger-warning.html
🧰 Additional context used
🔇 Additional comments (3)
_sass/_custom.scss (3)
207-209: LGTM: Modal backdrop opacity set appropriately.The opacity value of 0.8 for the modal backdrop provides a good balance, allowing some visibility of the underlying content while maintaining focus on the modal. This aligns well with the design discussion in the PR comments.
215-218: LGTM: Clear and contrasting selection style for trigger warning modal.The dark purple selection background (#3a1e3e) with white text provides excellent contrast, enhancing readability and user experience when selecting text within the trigger warning modal. The specific ID usage ensures this style is applied only where intended.
206-230: Overall assessment: Well-implemented styles for trigger warning feature.The new styles effectively implement the visual aspects of the trigger warning feature, aligning well with the PR objectives and previous discussions. The color scheme and design choices are appropriate for drawing attention to the warning without being overly alarming.
A few minor suggestions have been made to enhance accessibility and user interaction, particularly for the button styles. These small improvements could further refine the user experience.
To ensure consistency across the codebase, let's verify the usage of these new styles:
This script will help us confirm that the new styles are being applied correctly in the HTML templates.
Pull request
Proposed changes
First draft to solve #154
To discuss:
Types of changes
Checklist
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
sensible-inhalte.mdpage, previously outlining sensitive content warnings and related topics.