Skip to content
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

Merged
merged 2 commits into from
Jan 17, 2025
Merged

Hotfix/intervention-filter #2370

merged 2 commits into from
Jan 17, 2025

Conversation

shyambhongle
Copy link
Contributor

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

@shyambhongle shyambhongle requested a review from mohitb35 January 17, 2025 04:54
Copy link

vercel bot commented Jan 17, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
planet-webapp-multi-tenancy-setup ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 17, 2025 8:04am
2 Skipped Deployments
Name Status Preview Comments Updated (UTC)
planet-webapp ⬜️ Ignored (Inspect) Jan 17, 2025 8:04am
planet-webapp-temp ⬜️ Ignored (Inspect) Jan 17, 2025 8:04am

Copy link
Contributor

coderabbitai bot commented Jan 17, 2025

Walkthrough

This pull request introduces enhanced positioning capabilities for dropdown elements in the Projects V2 interface. The changes focus on adding new CSS classes in the InterventionList.module.scss file to provide more flexible vertical alignment options for dropdown buttons and intervention list options. The modifications extend to the InterventionList.tsx, index.tsx, and MapControls.tsx files, introducing a new optional hasProjectSites prop to conditionally apply these positioning classes.

Changes

File Change Summary
src/features/projectsV2/ProjectsMap/InterventionDropDown/InterventionList.module.scss Added 4 new CSS classes: .dropdownButtonAlignmentAbove, .dropdownButtonAlignmentBelow, .interventionListOptionsAbove, .interventionListOptionsBelow for flexible dropdown positioning
src/features/projectsV2/ProjectsMap/InterventionDropDown/InterventionList.tsx Added hasProjectSites optional prop to conditionally apply positioning classes to the list
src/features/projectsV2/ProjectsMap/InterventionDropDown/index.tsx Updated Props interface to include hasProjectSites prop for dropdown button positioning
src/features/projectsV2/ProjectsMap/MapControls.tsx Simplified rendering of dropdown components and passed hasProjectSites prop to InterventionDropDown

Possibly related PRs

Suggested labels

PR: reviewed-approved

Suggested reviewers

  • mohitb35
  • sunilsabatp

Poem

🐰 Dropdowns dance with grace and might,
Positioning classes take their flight!
Above or below, they now can sway,
Flexible UI finds its way!
A rabbit's code, both smart and light! 🎨


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?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR. (Beta)
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@shyambhongle shyambhongle changed the title Dynamic styling added Hotfix/intervention-filter Jan 17, 2025
Copy link
Contributor

@coderabbitai coderabbitai bot left a 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 and 10px 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

📥 Commits

Reviewing files that changed from the base of the PR and between 9db2a30 and 46658d7.

📒 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.scss

Length 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;
Copy link
Contributor

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

Copy link
Collaborator

@mohitb35 mohitb35 left a 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.

Copy link
Contributor

@mariahosfeld mariahosfeld left a 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.

Copy link
Contributor

@coderabbitai coderabbitai bot left a 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

📥 Commits

Reviewing files that changed from the base of the PR and between 46658d7 and bbb4598.

📒 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:

  1. The spacing aligns with the sites dropdown
  2. The calculations maintain visual hierarchy
  3. 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

Copy link
Collaborator

@mohitb35 mohitb35 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@mariahosfeld mariahosfeld merged commit 420384a into develop Jan 17, 2025
9 checks passed
@mariahosfeld mariahosfeld deleted the hotfix/intervention-filter branch January 17, 2025 09:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants