[723] Fix scroll after applying filters#179
Merged
Merged
Conversation
- Update filter anchor to use SCSS header height variables instead of hardcoded values - Add visibility hidden property to filter anchor for better accessibility - Restructure HTML layout by moving filter anchor outside main content area - Improve HTML formatting and code readability
Contributor
📝 Walkthrough## Walkthrough
This update introduces a new client-side filtering module for the case studies page, integrates its initialization into the main UI setup, and adjusts scrolling behavior for filter anchors. SCSS styles for case studies are enhanced for layout and transitions, while HTML and JavaScript files receive formatting and minor logic adjustments.
## Changes
| File(s) | Change Summary |
|-----------------------------------------------------------------------------------------|------------------------------------------------------------------------------------------------------------------|
| website/modules/asset/ui/src/clientSideFiltering.js | New module for client-side filtering: intercepts filter link clicks, updates URL, reloads page, handles popstate.|
| website/modules/asset/ui/src/index.js | Removes comments, integrates `initClientSideFiltering`, adjusts scroll-to-filter logic, minor whitespace tweaks. |
| website/modules/asset/ui/src/scss/_cases.scss | Adds min-height to `.cs_list`, uses header height vars, hides `.filter-anchor`, adds opacity transition to cards. |
| website/modules/case-studies-page/views/index.html | Formatting and indentation improvements; no logic or structural changes. |
## Sequence Diagram(s)
```mermaid
sequenceDiagram
participant User
participant Browser
participant ClientSideFiltering
participant Page
User->>Browser: Clicks filter link (industry/stack/caseStudyType)
Browser->>ClientSideFiltering: Intercepts click event
ClientSideFiltering-->>Browser: Prevents default navigation
ClientSideFiltering->>Browser: Updates URL with pushState
ClientSideFiltering->>Page: Reloads page to apply filter
Browser->>ClientSideFiltering: Detects popstate (back/forward)
ClientSideFiltering->>Page: Reloads page if state is client-side filterPossibly related PRs
Suggested reviewers
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
website/modules/asset/ui/src/scss/_cases.scss (1)
618-626: Consider using scroll-margin-top for anchor offset
Usingposition: relativewith negativetopoffsets works, but for scroll anchors it's more semantic and less layout-shifting to use thescroll-margin-topCSS property. For example:.filter-anchor { - position: relative; - top: -$mobile-header-height; + height: 0; + visibility: hidden; + scroll-margin-top: $mobile-header-height; @include breakpoint-medium { - top: -$desktop-header-height; + scroll-margin-top: $desktop-header-height; } }This approach keeps the anchor in flow and avoids manual positioning hacks.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
website/modules/asset/ui/src/scss/_cases.scss(1 hunks)website/modules/case-studies-page/views/index.html(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: lint
- GitHub Check: security-scan
- GitHub Check: e2e-tests
- GitHub Check: unit-tests
🔇 Additional comments (3)
website/modules/case-studies-page/views/index.html (3)
4-7: Approve: Multiline attributes for container<div>
Splitting theclassanddata-default-visible-tagsattributes onto separate lines improves readability and aligns with the template’s formatting conventions.
167-169: Approve: Multiline attributes for hidden tag list<li>
Movingclassanddata-labelto their own lines enhances clarity in the loop-generated tag markup.
197-200: Approve: Multiline attributes for “Show more”<button>
Separatingclass,data-filter-type, andtypeattributes makes the button markup more maintainable and easier to scan.
🔍 Vulnerabilities of
|
| digest | sha256:b41f8f49f7eb0196401ead601b8f48fe6919d921807f0d0d6958029aab672274 |
| vulnerabilities |     |
| platform | linux/amd64 |
| size | 290 MB |
| packages | 977 |
📦 Base Image node:23-alpine
| also known as |
|
| digest | sha256:b9d38d589853406ff0d4364f21969840c3e0397087643aef8eede40edbb6c7cd |
| vulnerabilities |   |
Description
| ||||||||||||
Description
| ||||||||||||
Description
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
website/modules/asset/ui/src/index.js(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: security-scan
- GitHub Check: unit-tests
- GitHub Check: e2e-tests
- GitHub Check: lint
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (actions)
🔇 Additional comments (2)
website/modules/asset/ui/src/index.js (2)
13-13: Minor formatting improvement.The added empty line improves code readability and structure.
299-299: ```shell
#!/bin/bashDisplay the context around both initSmoothCounters calls in index.js
echo "----- Lines 1–120 of index.js -----"
sed -n '1,120p' website/modules/asset/ui/src/index.jsecho "----- Lines 250–350 of index.js -----"
sed -n '250,350p' website/modules/asset/ui/src/index.js</details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
website/modules/asset/ui/src/clientSideFiltering.js (3)
3-15: Optimize the URL pattern matching logic.The current implementation has repetitive
href.includes()checks that could be simplified and made more maintainable.Consider refactoring to use a more efficient approach:
-const shouldInterceptClick = function (link) { - const href = link.getAttribute('href'); - return ( - href && - (href.includes('#filter') || - href.includes('?industry=') || - href.includes('&industry=') || - href.includes('?stack=') || - href.includes('&stack=') || - href.includes('?caseStudyType=') || - href.includes('&caseStudyType=')) - ); -}; +const shouldInterceptClick = function (link) { + const href = link.getAttribute('href'); + if (!href) return false; + + const filterParams = ['industry=', 'stack=', 'caseStudyType=']; + return href.includes('#filter') || + filterParams.some(param => href.includes(`?${param}`) || href.includes(`&${param}`)); +};
35-39: Use optional chaining as suggested by static analysis.The static analysis tool correctly identified that optional chaining can simplify the condition check.
Apply the suggested optional chaining:
const handlePopState = function (event) { - if (event.state && event.state.clientSideFilter) { + if (event.state?.clientSideFilter) { window.location.reload(); } };
41-48: Consider adding cleanup functionality.The current implementation adds event listeners but doesn't provide a way to remove them, which could lead to memory leaks in single-page applications or during testing.
Consider returning a cleanup function:
export const initClientSideFiltering = function () { if (!document.querySelector('.cs_list')) { - return; + return () => {}; // No-op cleanup } window.addEventListener('popstate', handlePopState); document.addEventListener('click', handleFilterClick); + + // Return cleanup function + return function cleanup() { + window.removeEventListener('popstate', handlePopState); + document.removeEventListener('click', handleFilterClick); + }; };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
website/modules/asset/ui/src/clientSideFiltering.js(1 hunks)website/modules/asset/ui/src/index.js(3 hunks)website/modules/asset/ui/src/scss/_cases.scss(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- website/modules/asset/ui/src/scss/_cases.scss
- website/modules/asset/ui/src/index.js
🧰 Additional context used
🪛 Biome (1.9.4)
website/modules/asset/ui/src/clientSideFiltering.js
[error] 36-36: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: e2e-tests
- GitHub Check: lint
- GitHub Check: unit-tests
- GitHub Check: security-scan
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (actions)
🔇 Additional comments (1)
website/modules/asset/ui/src/clientSideFiltering.js (1)
1-48: Overall implementation looks solid for the intended use case.The module correctly implements client-side filtering with page reload functionality. The approach of using
window.location.reload()after URL updates ensures server-side filtering logic is applied, which aligns with the PR objectives.
- Add error handling for URL construction in client-side filtering - Use optional chaining for safer event state checking - Refactor case studies anchor fix with destructured URL properties - Consolidate CSS rules for case study grid loading state - Remove redundant comments in index.js initialization
IhorMasechko
force-pushed
the
fix/avoid-reload-case-study-page-during-filtering
branch
from
e6c5f78 to
3ce3b57
Compare
yuramax
changed the title
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This fix ensures the filter anchor positioning is consistent across different screen sizes and improves the overall accessibility and maintainability of the case studies filtering functionality.