Made the list opened if tag is selected - #223
Conversation
🔍 Vulnerabilities of
|
| digest | sha256:35e5b4834a3e94aa5bfbef1f1866b475a2fcefa17aa24e96193ac76bc2433713 |
| vulnerabilities |     |
| platform | linux/amd64 |
| size | 291 MB |
| packages | 984 |
📦 Base Image node:23-alpine
| also known as |
|
| digest | sha256:b9d38d589853406ff0d4364f21969840c3e0397087643aef8eede40edbb6c7cd |
| vulnerabilities |     |
Description
| ||||||||||||
Description
| ||||||||||||
Description
| ||||||||||||
Description
| ||||||||||||
Description
|
WalkthroughRemoves sessionStorage-based persistence for expanded case-studies filter categories and replaces it with runtime, data-driven visibility in clientSideFiltering.js. Adds helpers (hasSelectedTagsInCategory, isDesktop), updateCategoriesVisibility and initializeCategoriesVisibility, a resize handler, and popstate handling; integrates visibility initialization into initClientSideFiltering and adds a change-event listener to keep aria-expanded in sync with checkbox state. Updates case-studies template to open a filter section when it is first or when that filter type has an active query. Adjusts modal panel positioning in SCSS from absolute-centered overlays to sticky, top-anchored panels. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches🧪 Generate unit tests
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. CodeRabbit Commands (Invoked using PR/Issue comments)Type 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: 4
🧹 Nitpick comments (4)
website/modules/asset/ui/src/clientSideFiltering.js (4)
28-31: Align desktop detection with CSS using matchMedia.Avoid off-by-one with >1024. Use the same breakpoint as CSS.
-const isDesktop = function () { - // Check if current viewport is desktop (typically > 1024px) - return window.innerWidth > 1024; -}; +const isDesktop = function () { + // Align with CSS breakpoint + return window.matchMedia('(min-width: 1024px)').matches; +};
33-61: Visibility logic is sound; consider minor cleanup.You re-query the button twice per branch; cache it once per checkbox to reduce DOM queries (tiny win).
- checkboxes.forEach(function (checkbox) { + checkboxes.forEach(function (checkbox) { const filterType = checkbox.id.replace('filter-toggle-', ''); const hasSelectedTags = hasSelectedTagsInCategory(filterType); const isIndustryCategory = filterType === 'industry'; + const button = document.querySelector(`label[for="${checkbox.id}"]`); // Industry category should always be open on desktop const shouldBeOpen = hasSelectedTags || (isIndustryCategory && isDesktop()); if (shouldBeOpen && !checkbox.checked) { checkbox.checked = true; - // Update aria-expanded attribute - const button = document.querySelector(`label[for="${checkbox.id}"]`); if (button) { button.setAttribute('aria-expanded', 'true'); } } else if (!shouldBeOpen && checkbox.checked) { checkbox.checked = false; - // Update aria-expanded attribute - const button = document.querySelector(`label[for="${checkbox.id}"]`); if (button) { button.setAttribute('aria-expanded', 'false'); } } });
120-127: Avoid brittle storage writes; simplify or guard.If you keep persistence, guard sessionStorage access; otherwise, remove this branch.
Option A (guard):
- if (filterLink.classList.contains('clear-all-link')) { - sessionStorage.removeItem(EXPANDED_CATEGORIES_KEY); - } else { - // Save current state of expanded categories before reload - saveExpandedCategories(); - } + try { + if (filterLink.classList.contains('clear-all-link')) { + sessionStorage.removeItem(EXPANDED_CATEGORIES_KEY); + } else { + saveExpandedCategories(); + } + } catch { /* storage may be unavailable */ }Option B (if removing persistence per earlier comment):
- // Clear saved state if this is a "clear all" action - if (filterLink.classList.contains('clear-all-link')) { - sessionStorage.removeItem(EXPANDED_CATEGORIES_KEY); - } else { - // Save current state of expanded categories before reload - saveExpandedCategories(); - } + // No-op; persistence removed
147-151: Debounce resize-driven DOM work.Resizing triggers frequent recalculations; debounce to reduce layout thrash on continuous resize.
-const handleResize = function () { - // Update categories visibility when viewport changes - updateCategoriesVisibility(); -}; +let resizeTid; +const handleResize = function () { + clearTimeout(resizeTid); + resizeTid = setTimeout(updateCategoriesVisibility, 150); +};
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
website/modules/asset/ui/src/clientSideFiltering.js(3 hunks)website/modules/case-studies-page/views/index.html(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: e2e-tests
- GitHub Check: unit-tests
- GitHub Check: security-scan
🔇 Additional comments (2)
website/modules/case-studies-page/views/index.html (1)
98-99: Server-side default-open logic aligns with PR goal.Opening a section when it has an active filter (or is first) matches “list opened if tag is selected.” No issues.
website/modules/asset/ui/src/clientSideFiltering.js (1)
28-31: Verify breakpoint parity with stylesI couldn’t locate any media query at 1024px in the CSS/SCSS or CSS-in-JS code. Please confirm that
window.innerWidth > 1024matches your actual filter‐panel breakpoints to avoid open/close mismatches at that boundary.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
website/modules/asset/ui/src/clientSideFiltering.js (3)
145-149: Add delegated change handler to keep aria-expanded in sync (and persist on toggle if keeping persistence).Without this, manual category toggles can desync aria-expanded; also enables full persistence path.
export const initClientSideFiltering = function () { if (!document.querySelector('.cs_list')) { return; } restoreExpandedCategories(); window.addEventListener('popstate', handlePopState); window.addEventListener('resize', handleResize); document.addEventListener('click', handleFilterClick); + document.addEventListener('change', function (e) { + const cb = e.target && e.target.closest('.filter-category__toggle'); + if (!cb) return; + const button = document.querySelector(`label[for="${cb.id}"]`); + if (button) { + button.setAttribute('aria-expanded', cb.checked ? 'true' : 'false'); + } + // Optional: persist manual expansions (if keeping persistence) + try { + const expanded = [...document.querySelectorAll('.filter-category__toggle:checked')] + .map(el => el.id.replace('filter-toggle-', '')); + sessionStorage.setItem(EXPANDED_CATEGORIES_KEY, JSON.stringify(expanded)); + } catch {} + }); };
3-19: Persistence is inconsistent with behavior; remove it or complete it.You save “expanded” categories but restore only when tags are selected, so manual expansions aren’t actually restored. Either drop persistence (simpler, matches PR scope “open if tag is selected”) or implement full persistence (restore regardless of selected tags and persist on toggle). Also guard sessionStorage.setItem to avoid QuotaExceeded errors.
Option A — remove persistence now:
-const EXPANDED_CATEGORIES_KEY = 'caseStudiesExpandedCategories'; - -const saveExpandedCategories = function () { - const expandedCategories = []; - const checkboxes = document.querySelectorAll('.filter-category__toggle'); - checkboxes.forEach(function (checkbox) { - if (checkbox.checked) { - const filterType = checkbox.id.replace('filter-toggle-', ''); - expandedCategories.push(filterType); - } - }); - - sessionStorage.setItem( - EXPANDED_CATEGORIES_KEY, - JSON.stringify(expandedCategories), - ); -}; +// Persistence removed: current requirement is “open if tag is selected”.Option B — keep persistence, but guard setItem:
- sessionStorage.setItem( - EXPANDED_CATEGORIES_KEY, - JSON.stringify(expandedCategories), - ); + try { + sessionStorage.setItem( + EXPANDED_CATEGORIES_KEY, + JSON.stringify(expandedCategories), + ); + } catch {}
56-86: Restore path doesn’t restore manual expansions; simplify or implement fully.Current restore only opens saved categories if they also have selected tags, so manual expansions never persist.
Option A — simplify to visibility-by-selection only:
-const restoreExpandedCategories = function () { - try { - const saved = sessionStorage.getItem(EXPANDED_CATEGORIES_KEY); - if (!saved) { - updateCategoriesVisibility(); - return; - } - - const expandedCategories = JSON.parse(saved); - expandedCategories.forEach(function (filterType) { - const checkbox = document.getElementById(`filter-toggle-${filterType}`); - if (checkbox && !checkbox.checked) { - if (hasSelectedTagsInCategory(filterType)) { - checkbox.checked = true; - const button = document.querySelector( - `label[for="filter-toggle-${filterType}"]`, - ); - if (button) { - button.setAttribute('aria-expanded', 'true'); - } - } - } - }); - - updateCategoriesVisibility(); - } catch (error) { - // Fallback to default visibility logic if parsing fails - console.warn('Failed to restore expanded categories:', error); - updateCategoriesVisibility(); - } -}; +const restoreExpandedCategories = function () { + updateCategoriesVisibility(); +};Option B — honor saved manual expansions in addition to “selected” and desktop industry:
-const restoreExpandedCategories = function () { - try { - const saved = sessionStorage.getItem(EXPANDED_CATEGORIES_KEY); - if (!saved) { - updateCategoriesVisibility(); - return; - } - - const expandedCategories = JSON.parse(saved); - expandedCategories.forEach(function (filterType) { - const checkbox = document.getElementById(`filter-toggle-${filterType}`); - if (checkbox && !checkbox.checked) { - if (hasSelectedTagsInCategory(filterType)) { - checkbox.checked = true; - const button = document.querySelector( - `label[for="filter-toggle-${filterType}"]`, - ); - if (button) { - button.setAttribute('aria-expanded', 'true'); - } - } - } - }); - - updateCategoriesVisibility(); - } catch (error) { - // Fallback to default visibility logic if parsing fails - console.warn('Failed to restore expanded categories:', error); - updateCategoriesVisibility(); - } -}; +const restoreExpandedCategories = function () { + try { + const saved = sessionStorage.getItem(EXPANDED_CATEGORIES_KEY); + const expanded = saved ? new Set(JSON.parse(saved)) : new Set(); + const checkboxes = document.querySelectorAll('.filter-category__toggle'); + checkboxes.forEach((checkbox) => { + const filterType = checkbox.id.replace('filter-toggle-', ''); + const shouldBeOpen = + expanded.has(filterType) || + hasSelectedTagsInCategory(filterType) || + (filterType === 'industry' && isDesktop()); + if (checkbox.checked !== shouldBeOpen) { + checkbox.checked = shouldBeOpen; + } + const button = document.querySelector(`label[for="${checkbox.id}"]`); + if (button) { + button.setAttribute('aria-expanded', shouldBeOpen ? 'true' : 'false'); + } + }); + } catch (error) { + console.warn('Failed to restore expanded categories:', error); + updateCategoriesVisibility(); + } +};
🧹 Nitpick comments (4)
website/modules/asset/ui/src/clientSideFiltering.js (4)
28-31: Align viewport check with CSS breakpoints via matchMedia.innerWidth thresholds easily drift from SCSS breakpoints; use matchMedia and confirm the exact min-width used in styles.
-const isDesktop = function () { - return window.innerWidth > 1024; -}; +const isDesktop = function () { + return window.matchMedia('(min-width: 1025px)').matches; +};Please verify the SCSS breakpoint value used for “desktop”.
32-54: Always sync aria-expanded, even when checked state doesn’t change.Currently aria only updates when you flip checked; if markup ships with mismatched aria, it stays stale. Update aria every pass and avoid duplicate querySelector calls.
-const updateCategoriesVisibility = function () { - const checkboxes = document.querySelectorAll('.filter-category__toggle'); - checkboxes.forEach(function (checkbox) { - const filterType = checkbox.id.replace('filter-toggle-', ''); - const hasSelectedTags = hasSelectedTagsInCategory(filterType); - const isIndustryCategory = filterType === 'industry'; - - const shouldBeOpen = hasSelectedTags || (isIndustryCategory && isDesktop()); - if (shouldBeOpen && !checkbox.checked) { - checkbox.checked = true; - const button = document.querySelector(`label[for="${checkbox.id}"]`); - if (button) { - button.setAttribute('aria-expanded', 'true'); - } - } else if (!shouldBeOpen && checkbox.checked) { - checkbox.checked = false; - const button = document.querySelector(`label[for="${checkbox.id}"]`); - if (button) { - button.setAttribute('aria-expanded', 'false'); - } - } - }); -}; +const updateCategoriesVisibility = function () { + const checkboxes = document.querySelectorAll('.filter-category__toggle'); + checkboxes.forEach(function (checkbox) { + const filterType = checkbox.id.replace('filter-toggle-', ''); + const shouldBeOpen = + hasSelectedTagsInCategory(filterType) || + (filterType === 'industry' && isDesktop()); + + if (checkbox.checked !== shouldBeOpen) { + checkbox.checked = shouldBeOpen; + } + const button = document.querySelector(`label[for="${checkbox.id}"]`); + if (button) { + button.setAttribute('aria-expanded', shouldBeOpen ? 'true' : 'false'); + } + }); +};
112-117: Guard storage ops and/or remove per Option A.If keeping persistence, wrap storage calls; if removing persistence, delete this block entirely.
Option A — remove:
- if (filterLink.classList.contains('clear-all-link')) { - sessionStorage.removeItem(EXPANDED_CATEGORIES_KEY); - } else { - saveExpandedCategories(); - }Option B — guard:
- if (filterLink.classList.contains('clear-all-link')) { - sessionStorage.removeItem(EXPANDED_CATEGORIES_KEY); - } else { - saveExpandedCategories(); - } + if (filterLink.classList.contains('clear-all-link')) { + try { sessionStorage.removeItem(EXPANDED_CATEGORIES_KEY); } catch {} + } else { + // saveExpandedCategories() already guards setItem in Option B above + saveExpandedCategories(); + }
136-139: Throttle resize to avoid excessive DOM scans.updateCategoriesVisibility() queries the DOM; throttle via rAF.
-const handleResize = function () { - updateCategoriesVisibility(); -}; +let __resizeRaf; +const handleResize = function () { + if (__resizeRaf) cancelAnimationFrame(__resizeRaf); + __resizeRaf = requestAnimationFrame(() => { + updateCategoriesVisibility(); + __resizeRaf = null; + }); +};
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
website/modules/asset/ui/src/clientSideFiltering.js(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: e2e-tests
- GitHub Check: security-scan
🔇 Additional comments (1)
website/modules/asset/ui/src/clientSideFiltering.js (1)
21-26: LGTM: active-tag detection is straightforward.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
website/modules/asset/ui/src/clientSideFiltering.js (1)
3-4: Persistence removal acknowledged and correct.Matches the PR scope “open if tag is selected”; no dead sessionStorage left.
🧹 Nitpick comments (6)
website/modules/asset/ui/src/clientSideFiltering.js (6)
5-10: Remove trailing comma for wider browser compatibility.Trailing commas in function calls can trip older runtimes; also simplifies diff noise.
- const selectedTags = document.querySelectorAll( - `#filter-content-${filterType} .tag-item.active`, - ); + const selectedTags = document.querySelectorAll( + `#filter-content-${filterType} .tag-item.active` + );Confirm that “.tag-item.active” is the authoritative active-state marker in the rendered HTML.
12-14: Align breakpoint detection with CSS using matchMedia.Avoids off-by-one mismatches with 1024/1025 CSS breakpoints.
-const isDesktop = function () { - return window.innerWidth > 1024; -}; +const isDesktop = function () { + return window.matchMedia('(min-width: 1025px)').matches; +};Please confirm your SCSS/CSS desktop breakpoint (1024 vs 1025) to ensure parity.
40-42: Tiny cleanup: inline initializer.initializeCategoriesVisibility is a single call-through; consider inlining to reduce indirection.
86-88: Throttle resize-driven DOM work.Prevent excessive DOM queries on rapid resizes.
-const handleResize = function () { - updateCategoriesVisibility(); -}; +const handleResize = function () { + if (resizeRafId) cancelAnimationFrame(resizeRafId); + resizeRafId = requestAnimationFrame(updateCategoriesVisibility); +};Add once at module scope:
let resizeRafId = null;Also applies to: 98-98
95-96: Make init idempotent to avoid duplicate listeners.Protect against double-initialization in CMS/partial reloads.
// module scope let clientFilteringInitialized = false; // inside initClientSideFiltering(), at the top: if (clientFilteringInitialized) return; clientFilteringInitialized = true;Is initClientSideFiltering guaranteed to be called exactly once per page view?
101-111: ARIA sync handler looks good.Delegated change listener correctly maintains aria-expanded on manual toggles.
Optionally preserve user intent across resizes by tracking manual toggles (e.g., set checkbox.dataset.manualOpen = 'true'/'false' here and have updateCategoriesVisibility skip overriding when this dataset is present). I can draft that follow-up if desired.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
website/modules/asset/ui/src/clientSideFiltering.js(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: e2e-tests
- GitHub Check: lint
- GitHub Check: unit-tests
- GitHub Check: security-scan
🔇 Additional comments (1)
website/modules/asset/ui/src/clientSideFiltering.js (1)
16-38: Ensure templates includedata-default-openon default categories
Add thedata-default-openattribute to the intended default-open section in your HTML (e.g. in website/modules/case-studies-page/views/index.html around line 120) so the JS can detect and apply desktop defaults correctly.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
website/modules/asset/ui/src/scss/_cases.scss (1)
1289-1296: Consistency with other sticky elementsOther sticky elements in this file use
$mobile-header-height/$desktop-header-height. Aligning this modal’s offset with those variables avoids header overlap and keeps behavior consistent.If you keep a constant offset, at least document why 90px is needed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
website/modules/asset/ui/src/scss/_cases.scss(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: e2e-tests
- GitHub Check: unit-tests
- GitHub Check: security-scan
- GitHub Check: lint
🔇 Additional comments (1)
website/modules/asset/ui/src/scss/_cases.scss (1)
1289-1296: Avoid horizontal overflow: left/right + 90vw + min-width can exceed 100vwWith
left: 20px; right: 20px; width: 90vw; min-width: 300px, the rendered width can exceed the viewport on 320px devices (e.g., 300px + 40px > 320px). Rely on left/right constraints (or a clamp) instead.Use one of:
- Keep
left/rightand remove bothwidthandmin-width(as in the diff above).- Or, if you prefer explicit width:
width: min(400px, calc(100vw - 40px));and dropmin-width.
Please verify on iPhone SE/Small Android widths and with long filter content.
2 participants
Made the list opened if tag is selected