[559] Create "Filter Case Studies" button for mobile version#163
Conversation
📝 WalkthroughWalkthroughA mobile filter modal feature for case studies was introduced. This includes a new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant FilterCTA as "Filter CTA Button"
participant FilterModal
participant DOM
participant Body
User->>FilterCTA: Click "Filter Case Studies"
FilterCTA->>FilterModal: open()
FilterModal->>DOM: Move filter elements into modal
FilterModal->>DOM: Add "active" class to modal
FilterModal->>Body: Disable scrolling
User->>FilterModal: Click close/backdrop/Escape
FilterModal->>DOM: Move filter elements back
FilterModal->>DOM: Remove "active" class from modal
FilterModal->>Body: Enable scrolling
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (5)
✨ Finishing Touches
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
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: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
website/public/images/arrow-back.svgis excluded by!**/*.svg
📒 Files selected for processing (3)
website/modules/asset/ui/src/index.js(1 hunks)website/modules/asset/ui/src/scss/_cases.scss(2 hunks)website/modules/case-studies-page/views/index.html(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: e2e-tests
- GitHub Check: security-scan
- GitHub Check: unit-tests
- GitHub Check: lint
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (actions)
🔇 Additional comments (11)
website/modules/asset/ui/src/scss/_cases.scss (4)
387-389: LGTM! Good use of responsive mixins.The transition from fixed font values to responsive mixins improves the scalability and maintainability of the typography system.
391-406: Well-styled filter CTA button with proper responsive behavior.The implementation correctly hides the button on medium+ breakpoints and provides appropriate styling for mobile interaction.
996-1013: Solid mobile-first approach for filter visibility.The media query logic properly manages the display state of filter elements, showing them only when the modal is active on small screens.
1015-1087: Comprehensive modal implementation with good UX considerations.The modal styles include:
- Proper z-index management (1000)
- Full viewport coverage with backdrop
- Centered, responsive content container
- Smooth transitions and accessibility-friendly styling
The pointer-events management ensures proper interaction states.
website/modules/case-studies-page/views/index.html (3)
12-12: Simple and effective filter CTA implementation.The button integrates seamlessly with the existing filter info layout and uses the appropriate CSS class for styling.
85-104: Excellent accessibility improvements for filter toggles.The restructured markup includes:
- Proper ARIA attributes (
aria-expanded,aria-controls,aria-labelledby)- Semantic label associations
- Keyboard interaction support with
onkeydown- Screen reader friendly structure
This significantly improves the user experience for assistive technology users.
251-264: Well-structured modal dialog with proper accessibility.The modal implementation includes:
- Semantic container structure
- Proper ARIA labeling for the close button
- Keyboard-accessible close button with
tabindex="0"- Visual back icon for intuitive navigation
- Empty body container for dynamic content injection
website/modules/asset/ui/src/index.js (4)
212-238: Well-designed constructor with proper initialization.The constructor effectively:
- Accepts a configuration object for flexibility
- Stores all necessary DOM references
- Maintains original parent relationships for restoration
- Calls initialization automatically
248-260: Good restoration logic with proper cleanup.The
close()method correctly restores elements to their original positions and cleans up the modal state. The order of operations is appropriate.
262-274: Comprehensive event handling with good UX patterns.The event listeners cover all expected interaction patterns:
- Click to open/close
- Backdrop click to close
- Escape key to close
- Proper conditional check for modal active state
240-246: 🛠️ Refactor suggestionConsider adding error handling for missing elements.
The
open()method assumes all elements exist. Consider adding null checks to prevent runtime errors when elements are missing.open() { this.modalBody.innerHTML = ''; - if (this.clearAll) this.modalBody.appendChild(this.clearAll); - if (this.selectedTags) this.modalBody.appendChild(this.selectedTags); - if (this.tagsFilter) this.modalBody.appendChild(this.tagsFilter); + if (this.clearAll && this.clearAll.parentNode) { + this.modalBody.appendChild(this.clearAll); + } + if (this.selectedTags && this.selectedTags.parentNode) { + this.modalBody.appendChild(this.selectedTags); + } + if (this.tagsFilter && this.tagsFilter.parentNode) { + this.modalBody.appendChild(this.tagsFilter); + } this.modal.classList.add('active'); }Likely an incorrect or invalid review comment.
🔍 Vulnerabilities of
|
| digest | sha256:e3287fab3e11d40753f4ce338a058a49d5cc05117badf853a0856c2088a5dd48 |
| vulnerabilities |     |
| platform | linux/amd64 |
| size | 289 MB |
| packages | 971 |
📦 Base Image node:23-alpine
| also known as |
|
| digest | sha256:b9d38d589853406ff0d4364f21969840c3e0397087643aef8eede40edbb6c7cd |
| vulnerabilities |  |
Description
| ||||||||||||
Description
| ||||||||||||
Description
|
…erModal class for handling mobile filter UI, initialize on DOM load and Barba transitions, support keyboard navigation
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (3)
website/modules/asset/ui/src/filterModal.js (2)
38-53: Improve readability of parent tracking logic.The conditional logic for storing original parents is repetitive and could be refactored for better maintainability.
- this.originalParents = {}; - if (this.clearAll) { - this.originalParents.clearAll = this.clearAll.parentNode; - } else { - this.originalParents.clearAll = null; - } - if (this.selectedTags) { - this.originalParents.selectedTags = this.selectedTags.parentNode; - } else { - this.originalParents.selectedTags = null; - } - if (this.tagsFilter) { - this.originalParents.tagsFilter = this.tagsFilter.parentNode; - } else { - this.originalParents.tagsFilter = null; - } + this.originalParents = { + clearAll: this.clearAll?.parentNode || null, + selectedTags: this.selectedTags?.parentNode || null, + tagsFilter: this.tagsFilter?.parentNode || null + };
109-113: Consider using AbortController for better event listener management.The escape key listener is added to the document without cleanup, which could lead to memory leaks if multiple instances are created.
+ this.abortController = new AbortController(); + document.addEventListener('keydown', (event) => { if (event.key === 'Escape' && this.modal.classList.contains('active')) { this.close(); } - }); + }, { signal: this.abortController.signal });Add a cleanup method to the class:
destroy() { this.abortController?.abort(); }website/modules/asset/ui/src/scss/_cases.scss (1)
392-400: Add focus management for screen readers.The mobile items count has proper visibility toggling, but consider adding
aria-liveregions for dynamic content updates..items-count__mobile { @include font-settings(10px, 140%, $font-weight-extra-bold); color: $gray-500; display: none; + + &[aria-live] { + position: absolute; + clip: rect(1px, 1px, 1px, 1px); + } &.is-visible { display: block; + position: static; + clip: auto; } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
website/modules/asset/ui/src/filterModal.js(1 hunks)website/modules/asset/ui/src/index.js(2 hunks)website/modules/asset/ui/src/scss/_cases.scss(3 hunks)website/modules/case-studies-page/views/index.html(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- website/modules/asset/ui/src/index.js
- website/modules/case-studies-page/views/index.html
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: lint
- GitHub Check: e2e-tests
- GitHub Check: unit-tests
- GitHub Check: security-scan
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (3)
website/modules/asset/ui/src/filterModal.js (2)
9-14: LGTM! Simple utility function is well-implemented.The function correctly creates and configures an h2 element for the modal header.
65-69:Details
❌ Incorrect review comment
Verify the items count element behavior.
The code assumes
.items-count__mobileexists and adds a visibility class, but doesn't handle the case where it might not exist initially or be moved multiple times.
🏁 Script executed:
#!/bin/bash # Verify items count mobile element usage in templates rg -A 3 -B 3 "items-count__mobile"Length of output: 2348
Items count behavior is already covered
- The
init()method safely checks for.items-count__mobilewithif (itemsCount)so it won’t error when the element is absent.- On close, the modal’s
close()handler removes theis-visibleclass and re-appends the element back to.cs_filter-info, restoring its original parent.- Using
appendChildmoves the node rather than cloning it, so repeated open/close cycles won’t produce duplicates.No additional handling is required here.
Likely an incorrect or invalid review comment.
website/modules/asset/ui/src/scss/_cases.scss (1)
388-390: LGTM! Responsive typography improvements.The responsive font and line-height mixins properly enhance the existing items count styling.
…itical DOM elements
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
website/modules/asset/ui/src/scss/_cases.scss (7)
402-426: Filter CTA button statesYou’ve styled the mobile “Filter” CTA with hover and focus transitions—good for visual feedback. For full accessibility, consider adding a
:focus-visibleoutline (instead of no outline) so keyboard users see a clear focus ring.
1015-1018: Reduced-motion: disable all transitionsYou’ve disabled backdrop transitions under
prefers-reduced-motion, but the modal content still animates. To fully respect user preferences, also disable contenttransition/animationhere.
1039-1048: Base modal container stylesThe fixed
.filter-modalwithpointer-events: noneandopacitytoggling lays a solid foundation. For a smoother open/close, you could addtransition: opacity 0.2s ease-in-outon the container itself.
1054-1062: Backdrop stylingThe semi-transparent backdrop with z-index layering is correct. To ensure clicks on the backdrop close the modal, you might want to explicitly set
pointer-events: autohere (the container handles events, but being explicit can prevent slip-through).
1080-1086: Close button stylingThe text-based close button is styled but lacks hover/focus feedback. Consider adding a subtle color change or underline on
:hover/:focusto make it more discoverable.
1088-1090: Modal body spacingThe
margin-top: 43pxon&__bodyworks, but consider using a spacing variable or consistent scale (e.g.$space-4) for maintainability.
1108-1111: Inactive modal stateYou correctly disable interactions and hide the modal when not active. For consistency, you might add a matching
transitionfor opacity here as well.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
website/modules/asset/ui/src/scss/_cases.scss(4 hunks)website/modules/asset/ui/src/scss/_variables.scss(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- website/modules/asset/ui/src/scss/_variables.scss
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Analyze (actions)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (11)
website/modules/asset/ui/src/scss/_cases.scss (11)
361-368: Align filter info items verticallyYou added
align-items: centerto.cs_filter-info, which improves vertical centering on mobile. This works well, but double-check that it doesn’t conflict with the desktop layout (where you switch toalign-items: flex-start).
383-391: Responsive typography for items countThe new
@include responsive-fontand@include responsive-line-heightmixins on.items-countensure legible text across breakpoints. Nice use of existing mixins.
392-400: Introduce mobile-only items count
.items-count__mobileprovides a clean way to show a smaller count in the modal. Ensure your JavaScript toggles the.is-visibleclass appropriately when the modal opens/closes.
1021-1027: Hide desktop filter elements on mobileHiding
.clear-all,.selected-tags, and.tags-filterbelow$breakpoint-mediumis spot on. This cleans up the UI until the modal is active.
1028-1036: Toggle filter elements in mobile modalThe
#filter-modal.activeselector shows the hidden elements when the modal is open. Verify that the markup indeed uses theid="filter-modal"and that there aren’t other elements with the same id. Consider switching to a data-attribute or class selector for flexibility.
1050-1052: Override tags-filter inside modalSetting
max-width: noneon.tags-filterwithin the modal ensures the filter list uses full width. This matches the design.
1063-1079: Modal content wrapperGood responsive sizing (
90vw, min/max widths) and scroll containment. This keeps the content centered and usable on all devices.
1092-1098: Filter headerThe header style (
border-bottom, font settings) is clear and aligns with the section design. No changes needed here.
1100-1106: Top-row layoutUsing flex with gap ensures the close button and clear actions distribute nicely. Looks good.
1113-1116: Active modal stateSwitching
pointer-eventsandopacityon.activeis concise and performant.
1119-1127: Back-iconYour
.back-iconstyling is comprehensive. It loads the SVG correctly and sizes it appropriately.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
website/modules/asset/ui/src/scss/_cases.scss (3)
403-422: ⚡️ Enhance.filters-ctaaccessibility
Great hover and focus effects—consider adding a:focus-visibleoutline to support keyboard users and/or ensure the element is a native<button>(or includesrole="button") for screen-reader compatibility..filters-cta { /* existing styles */ &:focus-visible { - /* no outline currently */ + outline: 2px solid $gray-400; + outline-offset: 2px; } }Also applies to: 425-426
1021-1039: 🔧 Avoid ID selectors in responsive rules
Scoping via#filter-modalmay be overly specific and conflict with other modals. Consider using the component class instead for lower specificity:- @media (max-width: $breakpoint-medium) { - #filter-modal { + @media (max-width: $breakpoint-medium) { + .filter-modal { &.active { /* ... */ } } }
1040-1121: 🎯 Ensure keyboard focus styles in the modal
The.filter-modal__closebutton and other interactive elements lack visible focus states. Add:focus-visiblestyles to the close button for clarity when navigating via keyboard:.filter-modal { &__close { /* existing styles */ + &:focus-visible { + outline: 2px solid $gray-400; + outline-offset: 2px; + } } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
website/modules/@apostrophecms/shared-constants/ui/src/index.js(1 hunks)website/modules/asset/ui/src/scss/_cases.scss(4 hunks)website/modules/case-studies-page/views/index.html(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- website/modules/@apostrophecms/shared-constants/ui/src/index.js
🚧 Files skipped from review as they are similar to previous changes (1)
- website/modules/case-studies-page/views/index.html
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: e2e-tests
- GitHub Check: lint
- GitHub Check: unit-tests
- GitHub Check: security-scan
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (6)
website/modules/asset/ui/src/scss/_cases.scss (6)
8-11: ✅ Improved vertical rhythm in.cs_content
Switching to a flex‐column layout with consistentgapand bottom margin strengthens the section’s vertical rhythm on mobile and desktop.
363-366: ✅ Center items in mobile filter bar
Addingalign-items: centerensures that filter info elements align properly on small screens before the medium‐breakpoint override.
389-390: ✅ Make count text responsive
Applyingresponsive-fontandresponsive-line-heightmixins helps.items-countscale appropriately across breakpoints.
393-401: 🎉 New mobile count variant
Introducing.items-count__mobilewith a toggled.is-visiblestate covers the use case of very small viewports. Ensure the JS correctly toggles.is-visibleonly on mobile.
1015-1018: ✅ Honor reduced‐motion for modal backdrop
Disabling backdrop transition underprefers-reduced-motionaligns with accessibility best practices.
1122-1130: ✅ Decorative back‐icon styling
The.back-iconbackground rules are well-defined. If it's purely decorative, ensure the markup usesaria-hidden="true".
IhorMasechko
left a comment
IhorMasechko
left a comment
There was a problem hiding this comment.
Got in console:
Uncaught Error: Modal element not found: #filter-modal
at new FilterM
odal
Please try to fix it.
yuramax
dismissed stale reviews from Anton-88, VitalyyP, and coderabbitai[bot]
via
a13dc37
|
Thanks, @IhorMasechko. The issue is fixed, please check again. |
yuramax
deleted the
559-create-filter-case-studies-button-for-mobile-version
branch
5 participants
This PR adds a filter button for case studies in the mobile version of the website.