-
Notifications
You must be signed in to change notification settings - Fork 16
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Hotfix/intervention-filter #2370
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
WalkthroughThis pull request introduces enhanced positioning capabilities for dropdown elements in the Projects V2 interface. The changes focus on adding new CSS classes in the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
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 (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (4)
src/features/projectsV2/ProjectsMap/MapControls.tsx (2)
134-141
: Consider extracting common props to avoid duplication.The InterventionDropDown props are duplicated between mobile and desktop views. Consider extracting them to a common object.
+ const interventionDropDownWithProjectProps = { + ...interventionDropDownProps, + hasProjectSites, + }; - <InterventionDropDown - {...interventionDropDownProps} - isMobile={isMobile} - hasProjectSites={hasProjectSites} - /> + <InterventionDropDown + {...interventionDropDownWithProjectProps} + isMobile={isMobile} + />
151-153
: Improve code formatting for better readability.The props spreading and line breaks could be formatted better.
- <InterventionDropDown {...interventionDropDownProps} hasProjectSites={hasProjectSites} - /> + <InterventionDropDown + {...interventionDropDownProps} + hasProjectSites={hasProjectSites} + />src/features/projectsV2/ProjectsMap/InterventionDropDown/InterventionList.module.scss (2)
21-27
: Consider using CSS custom properties for magic numbers.The values
75px
and10px
in the calculations could be extracted into CSS custom properties for better maintainability.+ $dropdownTopSpacingLarge: 75px; + $dropdownTopSpacingSmall: 10px; .dropdownButtonAlignmentAbove { - top: calc($layoutPaddingSide + 75px); + top: calc($layoutPaddingSide + $dropdownTopSpacingLarge); } .dropdownButtonAlignmentBelow { - top: calc($layoutPaddingSide + 10px); + top: calc($layoutPaddingSide + $dropdownTopSpacingSmall); }
86-92
: Consider using CSS custom properties for consistent spacing.Similar to the dropdown button, the list options use magic numbers that could be extracted into CSS custom properties.
+ $listOptionsTopSpacingSmall: 55px; + $listOptionsTopSpacingLarge: 116px; .interventionListOptionsAbove { - top: calc($layoutPaddingTop + 55px); + top: calc($layoutPaddingTop + $listOptionsTopSpacingSmall); } .interventionListOptionsBelow { - top: calc($layoutPaddingTop + 116px); + top: calc($layoutPaddingTop + $listOptionsTopSpacingLarge); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/features/projectsV2/ProjectsMap/InterventionDropDown/InterventionList.module.scss
(3 hunks)src/features/projectsV2/ProjectsMap/InterventionDropDown/InterventionList.tsx
(2 hunks)src/features/projectsV2/ProjectsMap/InterventionDropDown/index.tsx
(5 hunks)src/features/projectsV2/ProjectsMap/MapControls.tsx
(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (javascript)
🔇 Additional comments (4)
src/features/projectsV2/ProjectsMap/InterventionDropDown/InterventionList.tsx (2)
17-17
: LGTM! Type definition looks correct.The optional boolean prop
hasProjectSites
is properly typed.
33-33
: Consider mobile responsiveness impact.The conditional class application looks correct, but verify that the positioning works well on mobile devices, especially given the existing mobile-specific styles in the SCSS file.
Run this command to check for any mobile-specific styles that might conflict:
✅ Verification successful
Mobile styles are properly implemented ✅
The mobile-specific styles in
InterventionList.module.scss
handle positioning appropriately, with the dropdown being properly constrained and positioned on small screens. The conditional classes for above/below positioning don't conflict with the mobile layout.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for mobile-specific styles affecting positioning rg -l "@include.*Phone|@media.*max-width" src/features/projectsV2/ProjectsMap/Length of output: 469
Script:
#!/bin/bash # Check the content of InterventionList.module.scss focusing on mobile styles and positioning cat src/features/projectsV2/ProjectsMap/InterventionDropDown/InterventionList.module.scssLength of output: 3349
src/features/projectsV2/ProjectsMap/InterventionDropDown/index.tsx (2)
26-26
: LGTM! Prop handling is consistent.The
hasProjectSites
prop is properly typed and passed through the component.Also applies to: 36-36
64-64
: Verify dropdown positioning across all viewport sizes.The conditional class application for dropdown positioning looks correct, but ensure it works well across different screen sizes and device orientations.
Run this command to check for any viewport-specific styles:
✅ Verification successful
The dropdown positioning implementation is robust and well-designed
The code handles positioning correctly across viewport sizes:
- Desktop uses absolute positioning with proper z-index and offset calculations
- Mobile view (@include xsPhoneView) appropriately adjusts the layout, dimensions, and content display
- Both above/below variants are properly implemented with consistent spacing
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for viewport-specific styles in the dropdown components rg -A 3 "@media|@include.*view" src/features/projectsV2/ProjectsMap/InterventionDropDown/Length of output: 91
Script:
#!/bin/bash # Find and display the CSS module file content fd -e module.scss -e module.css . src/features/projectsV2/ProjectsMap/InterventionDropDown/ --exec cat {}Length of output: 3367
@@ -62,8 +70,7 @@ $layoutPaddingSide: 20px; | |||
font-size: $fontXSmall; | |||
position: absolute; | |||
z-index: 4; | |||
top: 140px; | |||
right: 30px; | |||
right: 30px; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codebase verification
Fix invalid z-index value and maintain current stacking hierarchy
- The z-index values follow a clear hierarchy (1-2 for base, 4 for dropdowns, 10-11 for overlays)
⚠️ Invalid value found in ProjectsMap.module.scss:z-index: 2px
(z-index doesn't accept units)
🔗 Analysis chain
Verify z-index stacking context.
The component uses z-index values. Ensure there's a clear stacking context hierarchy to prevent positioning issues.
Run this command to check for z-index usage:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for z-index declarations in the project
rg "z-index:" src/features/projectsV2/ProjectsMap/
Length of output: 974
src/features/projectsV2/ProjectsMap/InterventionDropDown/InterventionList.module.scss
Show resolved
Hide resolved
src/features/projectsV2/ProjectsMap/InterventionDropDown/InterventionList.module.scss
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@shyambhongle Minor style adjustments suggested, but looks good otherwise.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Functionality looks good, all projects now show the interventions filter, regardless of how many they already recorded.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/features/projectsV2/ProjectsMap/InterventionDropDown/InterventionList.module.scss (1)
73-73
: Align right positioning with layout system.The arbitrary
right: 30px
value should be calculated using the layout system variables for consistency.- right: 30px; + right: calc($layoutPaddingSide + 10px);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/features/projectsV2/ProjectsMap/InterventionDropDown/InterventionList.module.scss
(3 hunks)
🔇 Additional comments (3)
src/features/projectsV2/ProjectsMap/InterventionDropDown/InterventionList.module.scss (3)
16-16
: Fix invalid z-index value.The z-index value should be a unitless integer to maintain the established stacking hierarchy.
- z-index: 2; + z-index: 2;
86-92
: Review vertical spacing calculations.The gap between above/below variants (60px vs 124px) creates a 64px difference. This might cause alignment issues with the sites dropdown as mentioned in previous reviews.
Please verify:
- The spacing aligns with the sites dropdown
- The calculations maintain visual hierarchy
- The positioning is consistent across different viewport sizes
21-28
: Adjust vertical positioning calculations for consistency.The vertical spacing between variants (75px vs 10px) creates inconsistent positioning. Additionally, verify if
$layoutPaddingSide
is needed in these calculations.Run this script to analyze the usage of padding variables:
✅ Verification successful
Vertical positioning calculations appear to be intentionally different.
The varying offsets (75px vs 10px) seem deliberate to accommodate different dropdown positions (above/below). The
$layoutPaddingSide
variable is correctly used for horizontal positioning where needed. The implementation follows patterns found in related components like SiteDropdown.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for layoutPadding usage in related files rg "layoutPadding(Top|Side)" src/features/projectsV2/ProjectsMap/Length of output: 1375
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Fixes #
Changes in this pull request:
This PR addresses the issue where the intervention filter was not displayed for some projects. To resolve this, I moved the component outside the "hasProject" condition. Also, I added styles based on the same condition to adjust the filter's position.
@mohitb35