Feature/intervention #2343
Feature/intervention #2343
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
shyambhongle
force-pushed
the
feature/intervention
branch
from
53257dc to
43d3856
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
shyambhongle
force-pushed
the
feature/intervention
branch
from
5fb466c to
9f9021c
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
There was a problem hiding this comment.
@shyambhongle Added 2 points (1 minor point and one that should be addressed). Once done, LGTM.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (10)
src/features/projectsV2/ProjectsMap/ProjectSiteDropDown/index.tsx (1)
80-84: Consider decoupling the site and intervention dropdown interactions.The current implementation creates a tight coupling between the site and intervention dropdowns through a circular dependency:
- Site dropdown toggles affect intervention state via
disableInterventionFilter- Intervention state affects site dropdown via
disableInterventionMenuThis bi-directional coupling could lead to:
- Race conditions if both dropdowns update simultaneously
- Unexpected UX where one dropdown closes another
- Difficulty in maintaining and testing the interaction logic
Consider these alternatives:
- Use a shared dropdown manager to coordinate states
- Implement a unidirectional data flow where only one dropdown can affect the other
- Use a state machine to explicitly define valid state transitions
Would you like me to provide example implementations for any of these approaches?
Also applies to: 89-92
src/features/projectsV2/ProjectDetails/styles/PlantLocationInfo.module.scss (2)
29-36: Extract duplicate styles into a shared class.The
.interventionTitleclass duplicates styles from.treeCount. Consider extracting these common styles into a shared class to follow DRY principles.+.sharedTitleStyles { + font-weight: 700; + font-size: 14px; + color: rgba(47, 51, 54, 1); + span { + font-weight: 400; + } +} -.treeCount { - font-weight: 700; - font-size: 14px; - color: rgba(47, 51, 54, 1); - span { - font-weight: 400; - } +.treeCount { + @extend .sharedTitleStyles; } -.interventionTitle { - font-weight: 700; - font-size: 14px; - color: rgba(47, 51, 54, 1); - span { - font-weight: 400; - } +.interventionTitle { + @extend .sharedTitleStyles; }
210-220: Add container gaps and fix potential syntax issues.
- Add proper spacing between metadata items using gap property instead of margin
- Ensure all style declarations use semicolons
.interventionMetadataGroup { display: flex; flex-direction: column; + gap: 10px; } .interventionMetaItem { padding: 20px; background: rgba(0, 122, 73, 0.05); border-radius: 12px; - margin-bottom: 10px; }src/features/common/types/plantLocation.d.ts (3)
1-1: Use explicit type imports consistently.Good job using explicit type import for
InterventionTypes. For consistency, make all imports explicit type imports.-import { DateString } from './common'; -import { Links } from './payments'; -import { Polygon, Point } from 'geojson'; +import type { DateString } from './common'; +import type { Links } from './payments'; +import type { Polygon, Point } from 'geojson';
58-64: Review the overlap betweenOtherInterventionsandPlantLocationMulti.The
OtherInterventionsinterface shares several properties withPlantLocationMulti. Consider:
- Creating a common interface for shared properties
- Documenting why these properties are duplicated
+interface BaseInterventionLocation { + sampleTreeCount: number; + sampleInterventions: SamplePlantLocation[]; + plantedSpecies: PlantedSpecies[]; + geometry: Point | Polygon; +} -export interface OtherInterventions extends PlantLocationBase { +export interface OtherInterventions extends PlantLocationBase, BaseInterventionLocation { type: InterventionTypes; - sampleTreeCount: number; - sampleInterventions: SamplePlantLocation[]; - plantedSpecies: PlantedSpecies[]; - geometry: Point | Polygon; } -export interface PlantLocationMulti extends PlantLocationBase { +export interface PlantLocationMulti extends PlantLocationBase, BaseInterventionLocation { type: 'multi-tree-registration'; nextMeasurementDate: DateString | null; - sampleTreeCount: number; - sampleInterventions: SamplePlantLocation[]; - plantedSpecies: PlantedSpecies[]; originalGeometry: Polygon; - geometry: Point | Polygon; measurements: null; nextMeasurementDate: null; }
6-7: Address TODOs with clear action items.The TODOs indicate potential improvements needed:
- Update
PlantLocationBasetypes based on API response- Consider moving to planet-sdk
Let's track these tasks properly.
Would you like me to create GitHub issues to track these TODOs? This will help ensure they're not forgotten and can be properly prioritized.
src/features/projectsV2/ProjectDetails/index.tsx (2)
131-133: Extract complex conditions into helper functions.The non-plantation type check could be more readable if extracted into a helper function.
+const isNonPlantationTypeVisible = ( + hoveredLocation: PlantLocation | null, + selectedLocation: PlantLocation | null, + isMobile: boolean +): boolean => { + return ( + isNonPlantationType(hoveredLocation) || + (isNonPlantationType(selectedLocation) && !isMobile) + ); +}; -const shouldShowOtherIntervention = - isNonPlantationType(hoveredPlantLocation) || - (isNonPlantationType(selectedPlantLocation) && !isMobile); +const shouldShowOtherIntervention = isNonPlantationTypeVisible( + hoveredPlantLocation, + selectedPlantLocation, + isMobile +);
26-27: Group related imports together.Consider grouping the imports by type (types, components, utilities) and sorting them alphabetically within each group.
-import OtherInterventionInfo from './components/OtherInterventionInfo'; -import { isNonPlantationType } from '../../../utils/constants/intervention'; +// Components +import MultiPlantLocationInfo from './components/MultiPlantLocationInfo'; +import OtherInterventionInfo from './components/OtherInterventionInfo'; +import SinglePlantLocationInfo from './components/SinglePlantLocationInfo'; + +// Utils +import { isNonPlantationType } from '../../../utils/constants/intervention';src/features/projectsV2/ProjectsMap/index.tsx (2)
153-153: Remove commented-out code.This commented condition is no longer needed as the logic is now handled by the PLANTATION_TYPES constant.
234-245: Simplify the plantLocationInfo prop logic.The condition for plantLocationInfo can be simplified since we already know the type isn't in PLANTATION_TYPES.
- plantLocationInfo={ - selectedPlantLocation?.type !== 'single-tree-registration' && - selectedPlantLocation?.type !== 'multi-tree-registration' - ? selectedPlantLocation - : null - } + plantLocationInfo={selectedPlantLocation}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
src/features/common/types/plantLocation.d.ts(2 hunks)src/features/projectsV2/ProjectDetails/components/microComponents/InterventionHeader.tsx(1 hunks)src/features/projectsV2/ProjectDetails/components/microComponents/OtherInterventionHeader.tsx(1 hunks)src/features/projectsV2/ProjectDetails/components/microComponents/OtherInterventionMetadata.tsx(1 hunks)src/features/projectsV2/ProjectDetails/index.tsx(3 hunks)src/features/projectsV2/ProjectDetails/styles/PlantLocationInfo.module.scss(3 hunks)src/features/projectsV2/ProjectsMap/MapControls.tsx(5 hunks)src/features/projectsV2/ProjectsMap/ProjectSiteDropDown/index.tsx(4 hunks)src/features/projectsV2/ProjectsMap/index.tsx(4 hunks)src/features/projectsV2/ProjectsMap/microComponents/PlantLocations.tsx(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- src/features/projectsV2/ProjectDetails/components/microComponents/InterventionHeader.tsx
- src/features/projectsV2/ProjectDetails/components/microComponents/OtherInterventionHeader.tsx
- src/features/projectsV2/ProjectDetails/components/microComponents/OtherInterventionMetadata.tsx
- src/features/projectsV2/ProjectsMap/microComponents/PlantLocations.tsx
- src/features/projectsV2/ProjectsMap/MapControls.tsx
🔇 Additional comments (7)
src/features/projectsV2/ProjectsMap/ProjectSiteDropDown/index.tsx (1)
44-45: LGTM! Props interface is well-defined.The new props clearly indicate their purpose and types, maintaining good TypeScript practices.
src/features/projectsV2/ProjectDetails/styles/PlantLocationInfo.module.scss (3)
47-48: LGTM! Improved spacing between flex items.The addition of
gap: 10pximproves layout consistency.
50-59: Address dropdown height and mobile view issues.Per PR objectives, the intervention dropdown:
- Obscures the satellite switch feature
- Is taller than the site dropdown
- Overlaps with other dropdowns in mobile view
Add max-height and mobile-specific styles:
.otherInterventionGroup { display: flex; font-size: 12px; max-height: 300px; overflow-y: auto; @media (max-width: 768px) { max-height: 250px; position: relative; z-index: 1; } } .otherInterventionDetailsItem { padding: 20px; background: rgba(0, 122, 73, 0.05); border-radius: 12px; flex: 1; @media (max-width: 768px) { padding: 15px; } }
63-63: LGTM! Minor width adjustment.src/features/projectsV2/ProjectsMap/index.tsx (3)
27-28: LGTM! Imports are well organized.The imports are correctly placed and PLANTATION_TYPES has been moved to utils as requested.
Line range hint
153-171: Verify unselection behavior for intervention points.The current implementation only handles unselection for single and multi-tree locations. Consider adding similar unselection behavior for intervention points with Point geometry.
Let's verify the current behavior:
#!/bin/bash # Search for intervention point handling in the codebase ast-grep --pattern 'onClick($_) { $$$ if ($_ && $_.geometry.type === "Point") { $$$ } $$$ }'
183-186: Verify intervention visibility logic.Based on the PR feedback, the intervention dropdown doesn't display results for some projects. Consider adding a check to verify if the project supports interventions before showing the intervention UI.
Let's check how interventions are determined for projects:
| {shouldShowOtherIntervention ? ( | ||
| <OtherInterventionInfo | ||
| plantLocationInfo={ | ||
| selectedPlantLocation?.type !== 'single-tree-registration' && | ||
| selectedPlantLocation?.type !== 'multi-tree-registration' | ||
| ? selectedPlantLocation | ||
| : hoveredPlantLocation?.type !== 'single-tree-registration' && | ||
| hoveredPlantLocation?.type !== 'multi-tree-registration' | ||
| ? hoveredPlantLocation | ||
| : null | ||
| } | ||
| setSelectedSamplePlantLocation={setSelectedSamplePlantLocation} | ||
| isMobile={isMobile} | ||
| /> | ||
| ) : null} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Simplify complex conditional prop assignment.
The plantLocationInfo prop assignment is complex and hard to read. Consider extracting the logic into a helper function.
+const getInterventionLocationInfo = (
+ selected: PlantLocation | null,
+ hovered: PlantLocation | null
+): PlantLocation | null => {
+ if (
+ selected?.type !== 'single-tree-registration' &&
+ selected?.type !== 'multi-tree-registration'
+ ) {
+ return selected;
+ }
+ if (
+ hovered?.type !== 'single-tree-registration' &&
+ hovered?.type !== 'multi-tree-registration'
+ ) {
+ return hovered;
+ }
+ return null;
+};
{shouldShowOtherIntervention ? (
<OtherInterventionInfo
- plantLocationInfo={
- selectedPlantLocation?.type !== 'single-tree-registration' &&
- selectedPlantLocation?.type !== 'multi-tree-registration'
- ? selectedPlantLocation
- : hoveredPlantLocation?.type !== 'single-tree-registration' &&
- hoveredPlantLocation?.type !== 'multi-tree-registration'
- ? hoveredPlantLocation
- : null
- }
+ plantLocationInfo={getInterventionLocationInfo(
+ selectedPlantLocation,
+ hoveredPlantLocation
+ )}
setSelectedSamplePlantLocation={setSelectedSamplePlantLocation}
isMobile={isMobile}
/>
) : null}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| {shouldShowOtherIntervention ? ( | |
| <OtherInterventionInfo | |
| plantLocationInfo={ | |
| selectedPlantLocation?.type !== 'single-tree-registration' && | |
| selectedPlantLocation?.type !== 'multi-tree-registration' | |
| ? selectedPlantLocation | |
| : hoveredPlantLocation?.type !== 'single-tree-registration' && | |
| hoveredPlantLocation?.type !== 'multi-tree-registration' | |
| ? hoveredPlantLocation | |
| : null | |
| } | |
| setSelectedSamplePlantLocation={setSelectedSamplePlantLocation} | |
| isMobile={isMobile} | |
| /> | |
| ) : null} | |
| const getInterventionLocationInfo = ( | |
| selected: PlantLocation | null, | |
| hovered: PlantLocation | null | |
| ): PlantLocation | null => { | |
| if ( | |
| selected?.type !== 'single-tree-registration' && | |
| selected?.type !== 'multi-tree-registration' | |
| ) { | |
| return selected; | |
| } | |
| if ( | |
| hovered?.type !== 'single-tree-registration' && | |
| hovered?.type !== 'multi-tree-registration' | |
| ) { | |
| return hovered; | |
| } | |
| return null; | |
| }; | |
| {shouldShowOtherIntervention ? ( | |
| <OtherInterventionInfo | |
| plantLocationInfo={getInterventionLocationInfo( | |
| selectedPlantLocation, | |
| hoveredPlantLocation | |
| )} | |
| setSelectedSamplePlantLocation={setSelectedSamplePlantLocation} | |
| isMobile={isMobile} | |
| /> | |
| ) : null} |
| {shouldShowOtherIntervention ? ( | ||
| <OtherInterventionInfo | ||
| plantLocationInfo={ | ||
| selectedPlantLocation?.type !== 'single-tree-registration' && | ||
| selectedPlantLocation?.type !== 'multi-tree-registration' | ||
| ? selectedPlantLocation | ||
| : null | ||
| } | ||
| setSelectedSamplePlantLocation={setSelectedSamplePlantLocation} | ||
| isMobile={props.isMobile} | ||
| /> | ||
| ) : null} |
There was a problem hiding this comment.
💡 Codebase verification
Confirmed UI layout issues need attention
The codebase shows:
- Dropdowns have viewport-based height constraints (42vh and 58vh) but may still interfere with the satellite switch
- No specific mobile layout rules to prevent dropdown overlap
- While gap spacing exists, it may need adjustment for better component separation
Consider:
- Reducing
max-heightfor dropdowns (e.g., 35vh) - Adding mobile-specific media queries to handle dropdown positioning
- Reviewing container gaps for consistency
🔗 Analysis chain
Address UI layout concerns from PR feedback.
Several UI issues were reported:
- Intervention dropdown height obscures satellite switch
- Dropdowns overlap in mobile view
- Missing container gaps
Consider adding CSS classes or styles to:
- Limit dropdown height
- Adjust mobile layout to prevent overlapping
- Add appropriate spacing between containers
Let's check the current styling:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for related styles
rg -g "*.scss" -g "*.css" "dropdown|container|gap" src/features/projectsV2/
Length of output: 5890
Script:
#!/bin/bash
# Search for OtherInterventionInfo styles and height-related properties
rg -g "*.scss" -g "*.css" "OtherInterventionInfo|height|max-height" src/features/projectsV2/
# Also check the component file for inline styles
rg -g "*.tsx" "OtherInterventionInfo" src/features/projectsV2/
Length of output: 7157
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/features/projectsV2/ProjectDetails/components/OtherInterventionInfo.tsx (3)
36-100: MovecreateCardDatato a dedicated utility file.For better maintainability and reusability, consider moving this function to a separate utility file. This aligns with the single responsibility principle and makes the code more modular.
Create a new file
src/features/projectsV2/ProjectDetails/utils/metadata.ts:import { OtherInterventions } from '../../../common/types/plantLocation'; interface MetaDataValue { value: string; label: string; } interface PublicMetaData { [key: string]: string | MetaDataValue; } /** * Processes and cleans metadata from plant location information * @param plantLocationInfo - The plant location information containing metadata * @returns Array of cleaned key-value pairs */ export function cleanPublicMetadata(plantLocationInfo: OtherInterventions | null) { // Existing implementation... }
115-126: Simplify totalTreesCount calculation using optional chaining.The current implementation has unnecessary complexity and can be simplified.
const { totalTreesCount } = useMemo(() => { const totalTreesCount = - plantLocationInfo && - plantLocationInfo.plantedSpecies && - plantLocationInfo.plantedSpecies.length > 0 + plantLocationInfo?.plantedSpecies?.length > 0 ? plantLocationInfo.plantedSpecies.reduce( (sum, species: { treeCount: number }) => sum + species.treeCount, 0 ) : 0; return { totalTreesCount }; - }, [plantLocationInfo, plantLocationInfo?.type]); + }, [plantLocationInfo?.plantedSpecies]);🧰 Tools
🪛 Biome (1.9.4)
[error] 117-118: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
128-141: Simplify sampleInterventionSpeciesImages logic and add return type.The current implementation has unnecessary complexity and lacks a return type annotation.
- const sampleInterventionSpeciesImages = useMemo(() => { + const sampleInterventionSpeciesImages = useMemo((): Array<{ + id: string; + image: string; + description: string; + }> => { - if (plantLocationInfo && plantLocationInfo.sampleInterventions.length > 0) { - const result = plantLocationInfo.sampleInterventions && plantLocationInfo.sampleInterventions.map((item) => { + if (plantLocationInfo?.sampleInterventions?.length > 0) { + return plantLocationInfo.sampleInterventions.map((item) => { return { id: item.coordinates[0].id, - image: item.coordinates[0].image ?? '', + image: item.coordinates[0]?.image ?? '', description: tProjectDetails('sampleTreeTag', { tag: item.tag }), }; }); - return result; } + return []; }, [plantLocationInfo]);🧰 Tools
🪛 Biome (1.9.4)
[error] 131-138: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/features/projectsV2/ProjectDetails/components/OtherInterventionInfo.tsx(1 hunks)src/features/projectsV2/ProjectDetails/components/microComponents/InterventionHeader.tsx(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
src/features/projectsV2/ProjectDetails/components/OtherInterventionInfo.tsx
[error] 117-118: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 131-138: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 183-184: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (javascript)
🔇 Additional comments (1)
src/features/projectsV2/ProjectDetails/components/microComponents/InterventionHeader.tsx (1)
1-33: LGTM! Clean and well-structured component.The component is well-implemented with proper type definitions, clear props interface, and good separation of concerns.
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (2)
src/features/projectsV2/ProjectDetails/index.tsx (2)
131-134: 🛠️ Refactor suggestionMove isNonPlantationType check to utils file.
The
shouldShowOtherInterventionlogic should be moved to a utility function for better code organization.Create a new utility function in the utils file:
// utils/projectV2.ts export const shouldShowOtherIntervention = ( hoveredLocation: PlantLocation | null, selectedLocation: PlantLocation | null ): boolean => { return isNonPlantationType(hoveredLocation) || isNonPlantationType(selectedLocation); };
195-204: 🛠️ Refactor suggestionSimplify complex conditional prop assignment.
The prop assignments for
OtherInterventionInfoare complex and hard to read. Consider extracting the logic into a helper function.+const getInterventionLocationInfo = ( + location: PlantLocation | null, +): PlantLocation | null => { + if (!location) return null; + return location.type !== 'single-tree-registration' && + location.type !== 'multi-tree-registration' + ? location + : null; +}; {shouldShowOtherIntervention ? ( <OtherInterventionInfo - selectedPlantLocation={selectedPlantLocation && selectedPlantLocation?.type !== 'single-tree-registration' && - selectedPlantLocation?.type !== 'multi-tree-registration' ? selectedPlantLocation : null} - hoveredPlantLocation={hoveredPlantLocation && hoveredPlantLocation?.type !== 'single-tree-registration' && - hoveredPlantLocation?.type !== 'multi-tree-registration' ? hoveredPlantLocation : null} + selectedPlantLocation={getInterventionLocationInfo(selectedPlantLocation)} + hoveredPlantLocation={getInterventionLocationInfo(hoveredPlantLocation)} setSelectedSamplePlantLocation={setSelectedSamplePlantLocation} isMobile={isMobile} /> ) : null}
🧹 Nitpick comments (2)
src/features/projectsV2/ProjectDetails/components/OtherInterventionInfo.tsx (2)
117-128: Optimize thetotalTreesCountcalculation.Simplify the logic using optional chaining and remove unnecessary dependency.
const { totalTreesCount } = useMemo(() => { const totalTreesCount = - plantLocationInfo && - plantLocationInfo.plantedSpecies && - plantLocationInfo.plantedSpecies.length > 0 + plantLocationInfo?.plantedSpecies?.length > 0 ? plantLocationInfo.plantedSpecies.reduce( (sum, species: { treeCount: number }) => sum + species.treeCount, 0 ) : 0; return { totalTreesCount }; - }, [plantLocationInfo, plantLocationInfo?.type]); + }, [plantLocationInfo?.plantedSpecies]);🧰 Tools
🪛 Biome (1.9.4)
[error] 119-120: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
130-143: Simplify thesampleInterventionSpeciesImageslogic.The current implementation has unnecessary complexity and potential undefined access.
const sampleInterventionSpeciesImages = useMemo(() => { - if (plantLocationInfo && plantLocationInfo.sampleInterventions.length > 0) { - const result = - plantLocationInfo.sampleInterventions && - plantLocationInfo.sampleInterventions.map((item) => { + if (plantLocationInfo?.sampleInterventions?.length > 0) { + return plantLocationInfo.sampleInterventions.map((item) => { return { id: item.coordinates[0].id, - image: item.coordinates[0].image ?? '', + image: item.coordinates[0]?.image ?? '', description: tProjectDetails('sampleTreeTag', { tag: item.tag }), }; }); - return result; } + return []; }, [plantLocationInfo]);🧰 Tools
🪛 Biome (1.9.4)
[error] 133-140: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
public/static/locales/en/projectDetails.jsonis excluded by!**/*.json
📒 Files selected for processing (6)
src/features/projectsV2/ProjectDetails/components/OtherInterventionInfo.tsx(1 hunks)src/features/projectsV2/ProjectDetails/components/OtherInterventionTitle.tsx(1 hunks)src/features/projectsV2/ProjectDetails/components/microComponents/InterventionHeader.tsx(1 hunks)src/features/projectsV2/ProjectDetails/index.tsx(3 hunks)src/features/projectsV2/ProjectsMap/InterventionDropDown/InterventionList.tsx(1 hunks)src/features/projectsV2/ProjectsMap/index.tsx(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/features/projectsV2/ProjectsMap/index.tsx
🧰 Additional context used
🪛 GitHub Check: CodeFactor
src/features/projectsV2/ProjectDetails/components/OtherInterventionTitle.tsx
[notice] 3-3: src/features/projectsV2/ProjectDetails/components/OtherInterventionTitle.tsx#L3
'AllInterventions' is defined but never used. Allowed unused vars must match /^_/u. (@typescript-eslint/no-unused-vars)
[notice] 14-62: src/features/projectsV2/ProjectDetails/components/OtherInterventionTitle.tsx#L14-L62
Complex Method
[notice] 3-3: src/features/projectsV2/ProjectDetails/components/OtherInterventionTitle.tsx#L3
'findInterventionHeader' is defined but never used. Allowed unused vars must match /^_/u. (@typescript-eslint/no-unused-vars)
[notice] 2-2: src/features/projectsV2/ProjectDetails/components/OtherInterventionTitle.tsx#L2
'INTERVENTION_TYPE' is defined but never used. Allowed unused vars must match /^_/u. (@typescript-eslint/no-unused-vars)
src/features/projectsV2/ProjectDetails/components/microComponents/InterventionHeader.tsx
[notice] 3-3: src/features/projectsV2/ProjectDetails/components/microComponents/InterventionHeader.tsx#L3
'findInterventionHeader' is defined but never used. Allowed unused vars must match /^_/u. (@typescript-eslint/no-unused-vars)
🪛 Biome (1.9.4)
src/features/projectsV2/ProjectDetails/components/OtherInterventionInfo.tsx
[error] 119-120: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 133-140: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 185-186: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (javascript)
🔇 Additional comments (4)
src/features/projectsV2/ProjectDetails/components/microComponents/InterventionHeader.tsx (1)
20-21: Update class names to match component purpose.The class names still reference plant location and planting details, which don't align with the intervention context.
- className={`plant-location-header-container ${styles.plantLocationHeaderContainer}`} + className={`intervention-header-container ${styles.interventionHeaderContainer}`} - className={`planting-details-item ${styles.plantingDetailsItem}`} + className={`intervention-details-item ${styles.interventionDetailsItem}`}Also applies to: 26-27
src/features/projectsV2/ProjectsMap/InterventionDropDown/InterventionList.tsx (2)
24-27: Rename handler function to match its purpose.The function name
handleFilterSelectionshould be renamed to better reflect its purpose of handling intervention selection.- const handleFilterSelection = (key: INTERVENTION_TYPE) => { + const handleInterventionSelection = (key: INTERVENTION_TYPE) => { setIsMenuOpen(false); setSelectedInterventionType(key); };
30-47: Improve accessibility and list rendering.The list implementation needs improvements for accessibility and React best practices:
- Using index as key in map function
- Missing accessibility attributes
- Missing keyboard navigation
- <ul className={styles.interventionListOptions}> + <ul className={styles.interventionListOptions} role="listbox"> {interventionList.map((intervention, index) => { return ( <li className={`${styles.listItem} ${intervention.value === selectedInterventionData?.value ? styles.selectedItem : ''}`} - onClick={() => handleFilterSelection(intervention.value)} - key={index} + onClick={() => handleInterventionSelection(intervention.value)} + key={intervention.value} + role="option" + aria-selected={intervention.value === selectedInterventionData?.value} + tabIndex={0} + onKeyDown={(e) => { + if (e.key === 'Enter' || e.key === ' ') { + handleInterventionSelection(intervention.value); + } + }} > <p> <OtherInterventionTitle type={intervention.value} /> </p> </li> ); })} </ul>src/features/projectsV2/ProjectDetails/components/OtherInterventionInfo.tsx (1)
196-208: Address mobile UI layout issues.Based on past review comments, there are issues with the mobile layout where dropdowns overlap and heights need adjustment.
Please verify the mobile layout by:
- Testing different screen sizes
- Ensuring dropdowns don't overlap
- Adjusting container heights appropriately
Consider adding responsive design utilities or CSS variables to manage component heights consistently across different views.
| import React from "react"; | ||
| import type { INTERVENTION_TYPE } from "../../../../utils/constants/intervention"; | ||
| import { AllInterventions, findInterventionHeader } from "../../../../utils/constants/intervention"; | ||
| import { useTranslations } from "next-intl"; |
There was a problem hiding this comment.
Remove unused imports to improve code maintainability.
The following imports are defined but never used:
INTERVENTION_TYPEAllInterventionsfindInterventionHeader
import React from "react";
-import type { INTERVENTION_TYPE } from "../../../../utils/constants/intervention";
-import { AllInterventions, findInterventionHeader } from "../../../../utils/constants/intervention";
import { useTranslations } from "next-intl";📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import React from "react"; | |
| import type { INTERVENTION_TYPE } from "../../../../utils/constants/intervention"; | |
| import { AllInterventions, findInterventionHeader } from "../../../../utils/constants/intervention"; | |
| import { useTranslations } from "next-intl"; | |
| import React from "react"; | |
| import { useTranslations } from "next-intl"; |
🧰 Tools
🪛 GitHub Check: CodeFactor
[notice] 3-3: src/features/projectsV2/ProjectDetails/components/OtherInterventionTitle.tsx#L3
'AllInterventions' is defined but never used. Allowed unused vars must match /^_/u. (@typescript-eslint/no-unused-vars)
[notice] 3-3: src/features/projectsV2/ProjectDetails/components/OtherInterventionTitle.tsx#L3
'findInterventionHeader' is defined but never used. Allowed unused vars must match /^_/u. (@typescript-eslint/no-unused-vars)
[notice] 2-2: src/features/projectsV2/ProjectDetails/components/OtherInterventionTitle.tsx#L2
'INTERVENTION_TYPE' is defined but never used. Allowed unused vars must match /^_/u. (@typescript-eslint/no-unused-vars)
| default: | ||
| '' | ||
| break; | ||
| } |
There was a problem hiding this comment.
Fix the default case in switch statement.
The default case has an unreachable break statement after an empty string literal.
default:
- ''
- break;
+ return '';📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| default: | |
| '' | |
| break; | |
| } | |
| default: | |
| return ''; | |
| } |
| const selectDynamicString = () => { | ||
| switch (type) { | ||
| case 'all': | ||
| return tProjectDetails('all'); | ||
| case 'default': | ||
| return tProjectDetails('default'); | ||
| case 'single-tree-registration': | ||
| return tProjectDetails('single-tree-registration'); | ||
| case 'multi-tree-registration': | ||
| return tProjectDetails('multi-tree-registration'); | ||
| case 'fire-suppression': | ||
| return tProjectDetails('fire-suppression'); | ||
| case 'soil-improvement': | ||
| return tProjectDetails('soil-improvement'); | ||
| case 'stop-tree-harvesting': | ||
| return tProjectDetails('stop-tree-harvesting'); | ||
| case 'removal-invasive-species': | ||
| return tProjectDetails('removal-invasive-species'); | ||
| case 'assisting-seed-rain': | ||
| return tProjectDetails('assisting-seed-rain'); | ||
| case 'fencing': | ||
| return tProjectDetails('fencing'); | ||
| case 'grass-suppression': | ||
| return tProjectDetails('grass-suppression'); | ||
| case 'direct-seeding': | ||
| return tProjectDetails('direct-seeding'); | ||
| case 'enrichment-planting': | ||
| return tProjectDetails('enrichment-planting'); | ||
| case 'firebreaks': | ||
| return tProjectDetails('firebreaks'); | ||
| case 'fire-patrol': | ||
| return tProjectDetails('fire-patrol'); | ||
| case 'liberating-regenerant': | ||
| return tProjectDetails('liberating-regenerant'); | ||
| case 'maintenance': | ||
| return tProjectDetails('maintenance'); | ||
| case 'marking-regenerant': | ||
| return tProjectDetails('marking-regenerant'); | ||
| case 'other-intervention': | ||
| return tProjectDetails('other-intervention'); | ||
| default: | ||
| '' | ||
| break; | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor the switch statement to reduce complexity.
The selectDynamicString function contains a large switch statement that could be simplified using a mapping object.
const OtherInterventionTitle = ({ type }: Props) => {
const tProjectDetails = useTranslations('ProjectDetails.intervention');
- const selectDynamicString = () => {
- switch (type) {
- case 'all':
- return tProjectDetails('all');
- case 'default':
- return tProjectDetails('default');
- // ... more cases
- default:
- ''
- break;
- }
- }
+ const interventionTypes = {
+ 'all': 'all',
+ 'default': 'default',
+ 'single-tree-registration': 'single-tree-registration',
+ 'multi-tree-registration': 'multi-tree-registration',
+ 'fire-suppression': 'fire-suppression',
+ 'soil-improvement': 'soil-improvement',
+ 'stop-tree-harvesting': 'stop-tree-harvesting',
+ 'removal-invasive-species': 'removal-invasive-species',
+ 'assisting-seed-rain': 'assisting-seed-rain',
+ 'fencing': 'fencing',
+ 'grass-suppression': 'grass-suppression',
+ 'direct-seeding': 'direct-seeding',
+ 'enrichment-planting': 'enrichment-planting',
+ 'firebreaks': 'firebreaks',
+ 'fire-patrol': 'fire-patrol',
+ 'liberating-regenerant': 'liberating-regenerant',
+ 'maintenance': 'maintenance',
+ 'marking-regenerant': 'marking-regenerant',
+ 'other-intervention': 'other-intervention'
+ } as const;
+ const selectDynamicString = () => type in interventionTypes ? tProjectDetails(interventionTypes[type as keyof typeof interventionTypes]) : '';📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const selectDynamicString = () => { | |
| switch (type) { | |
| case 'all': | |
| return tProjectDetails('all'); | |
| case 'default': | |
| return tProjectDetails('default'); | |
| case 'single-tree-registration': | |
| return tProjectDetails('single-tree-registration'); | |
| case 'multi-tree-registration': | |
| return tProjectDetails('multi-tree-registration'); | |
| case 'fire-suppression': | |
| return tProjectDetails('fire-suppression'); | |
| case 'soil-improvement': | |
| return tProjectDetails('soil-improvement'); | |
| case 'stop-tree-harvesting': | |
| return tProjectDetails('stop-tree-harvesting'); | |
| case 'removal-invasive-species': | |
| return tProjectDetails('removal-invasive-species'); | |
| case 'assisting-seed-rain': | |
| return tProjectDetails('assisting-seed-rain'); | |
| case 'fencing': | |
| return tProjectDetails('fencing'); | |
| case 'grass-suppression': | |
| return tProjectDetails('grass-suppression'); | |
| case 'direct-seeding': | |
| return tProjectDetails('direct-seeding'); | |
| case 'enrichment-planting': | |
| return tProjectDetails('enrichment-planting'); | |
| case 'firebreaks': | |
| return tProjectDetails('firebreaks'); | |
| case 'fire-patrol': | |
| return tProjectDetails('fire-patrol'); | |
| case 'liberating-regenerant': | |
| return tProjectDetails('liberating-regenerant'); | |
| case 'maintenance': | |
| return tProjectDetails('maintenance'); | |
| case 'marking-regenerant': | |
| return tProjectDetails('marking-regenerant'); | |
| case 'other-intervention': | |
| return tProjectDetails('other-intervention'); | |
| default: | |
| '' | |
| break; | |
| } | |
| } | |
| const interventionTypes = { | |
| 'all': 'all', | |
| 'default': 'default', | |
| 'single-tree-registration': 'single-tree-registration', | |
| 'multi-tree-registration': 'multi-tree-registration', | |
| 'fire-suppression': 'fire-suppression', | |
| 'soil-improvement': 'soil-improvement', | |
| 'stop-tree-harvesting': 'stop-tree-harvesting', | |
| 'removal-invasive-species': 'removal-invasive-species', | |
| 'assisting-seed-rain': 'assisting-seed-rain', | |
| 'fencing': 'fencing', | |
| 'grass-suppression': 'grass-suppression', | |
| 'direct-seeding': 'direct-seeding', | |
| 'enrichment-planting': 'enrichment-planting', | |
| 'firebreaks': 'firebreaks', | |
| 'fire-patrol': 'fire-patrol', | |
| 'liberating-regenerant': 'liberating-regenerant', | |
| 'maintenance': 'maintenance', | |
| 'marking-regenerant': 'marking-regenerant', | |
| 'other-intervention': 'other-intervention' | |
| } as const; | |
| const selectDynamicString = () => type in interventionTypes ? tProjectDetails(interventionTypes[type as keyof typeof interventionTypes]) : ''; |
🧰 Tools
🪛 GitHub Check: CodeFactor
[notice] 14-62: src/features/projectsV2/ProjectDetails/components/OtherInterventionTitle.tsx#L14-L62
Complex Method
| import styles from '../../styles/PlantLocationInfo.module.scss'; | ||
| import { formatHid } from '../../../../../utils/projectV2'; | ||
| import { findInterventionHeader } from '../../../../../utils/constants/intervention'; |
There was a problem hiding this comment.
Remove unused import and update import path.
The findInterventionHeader import is not used in the component.
import styles from '../../styles/PlantLocationInfo.module.scss';
import { formatHid } from '../../../../../utils/projectV2';
-import { findInterventionHeader } from '../../../../../utils/constants/intervention';📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import styles from '../../styles/PlantLocationInfo.module.scss'; | |
| import { formatHid } from '../../../../../utils/projectV2'; | |
| import { findInterventionHeader } from '../../../../../utils/constants/intervention'; | |
| import styles from '../../styles/PlantLocationInfo.module.scss'; | |
| import { formatHid } from '../../../../../utils/projectV2'; |
🧰 Tools
🪛 GitHub Check: CodeFactor
[notice] 3-3: src/features/projectsV2/ProjectDetails/components/microComponents/InterventionHeader.tsx#L3
'findInterventionHeader' is defined but never used. Allowed unused vars must match /^_/u. (@typescript-eslint/no-unused-vars)
| const createCardData = (plantLocationInfo: OtherInterventions | null) => { | ||
| // Initialize an array to store the cleaned key-value pairs | ||
| const cleanedData: { key: string; value: string }[] = []; | ||
|
|
||
| // Extract metadata from the plantLocationInfo object, if it exists | ||
| const parsedData = plantLocationInfo?.metadata; | ||
|
|
||
| // Check if `parsedData.public` exists, is an object, and is not an array | ||
| if ( | ||
| parsedData?.public && | ||
| typeof parsedData.public === 'object' && | ||
| !Array.isArray(parsedData.public) | ||
| ) { | ||
| // Iterate over the entries of `parsedData.public` as key-value pairs | ||
| Object.entries(parsedData.public as PublicMetaData).forEach( | ||
| ([key, value]) => { | ||
| // Skip the entry if the key is 'isEntireSite' as it's used to show point location and no use to user | ||
| if (key !== 'isEntireSite') { | ||
| // If the value is a string, directly add it to cleanedData | ||
| if (typeof value === 'string') { | ||
| cleanedData.push({ value, key }); | ||
| } | ||
| // If the value is an object with `value` and `label` properties | ||
| else if ( | ||
| typeof value === 'object' && | ||
| value !== null && | ||
| 'value' in value && | ||
| 'label' in value | ||
| ) { | ||
| // Check if the `value` property contains a valid JSON string | ||
| if (isJsonString(value.value)) { | ||
| try { | ||
| // Parse the JSON string | ||
| const parsedValue = JSON.parse(value.value); | ||
| // If the parsed value is an object with a `value` property, add it to cleanedData | ||
| if ( | ||
| parsedValue && | ||
| typeof parsedValue === 'object' && | ||
| 'value' in parsedValue | ||
| ) { | ||
| cleanedData.push({ | ||
| key: value.label, // Use the `label` property as the key | ||
| value: parsedValue.value, // Use the parsed `value` property | ||
| }); | ||
| } | ||
| } catch (error) { | ||
| // Log an error if JSON parsing fails | ||
| console.error('Error parsing JSON:', error); | ||
| } | ||
| } else { | ||
| // If not a JSON string, add the `label` and `value` directly | ||
| cleanedData.push({ | ||
| key: value.label, | ||
| value: value.value, | ||
| }); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| ); | ||
| } | ||
|
|
||
| // Return the array of cleaned key-value pairs | ||
| return cleanedData; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Move createCardData to a dedicated utility file.
For better maintainability and reusability, move this function to a separate utility file. This aligns with the single responsibility principle and makes the code more modular.
Create a new file src/features/projectsV2/ProjectDetails/utils/metadata.ts:
import { OtherInterventions } from '../../../common/types/plantLocation';
interface MetaDataValue {
value: string;
label: string;
}
interface PublicMetaData {
[key: string]: string | MetaDataValue;
}
/**
* Processes and cleans metadata from plant location information
* @param plantLocationInfo - The plant location information containing metadata
* @returns Array of cleaned key-value pairs with metadata
*/
export function cleanPublicMetadata(plantLocationInfo: OtherInterventions | null) {
// Move existing implementation here...
}| const content = [ | ||
| <> | ||
| <InterventionHeader | ||
| plHid={plantLocationInfo?.hid} | ||
| interventionType={plantLocationInfo?.type} | ||
| plantDate={plantLocationInfo?.interventionStartDate} | ||
| key="interventionHeader" | ||
| /> | ||
| {shouldDisplayImageCarousel && ( | ||
| <ImageSlider | ||
| key="imageSlider" | ||
| images={sampleInterventionSpeciesImages} | ||
| type="coordinate" | ||
| isMobile={isMobile} | ||
| imageSize="large" | ||
| allowFullView={!isMobile} | ||
| /> | ||
| )} | ||
| </>, | ||
| cleanedPublicMetadata.length > 0 && ( | ||
| <OtherInterventionMetadata | ||
| key="plantingDetails" | ||
| metadata={cleanedPublicMetadata} | ||
| plantDate={plantLocationInfo?.interventionStartDate} | ||
| type={plantLocationInfo?.type} | ||
| />), | ||
| plantLocationInfo?.plantedSpecies && | ||
| plantLocationInfo.plantedSpecies.length > 0 && ( | ||
| <SpeciesPlanted | ||
| key="speciesPlanted" | ||
| totalTreesCount={totalTreesCount} | ||
| plantedSpecies={plantLocationInfo.plantedSpecies} | ||
| /> | ||
| ), | ||
| plantLocationInfo && | ||
| plantLocationInfo.sampleInterventions && | ||
| plantLocationInfo.sampleInterventions.length > 0 && ( | ||
| <SampleTrees | ||
| key="sampleTrees" | ||
| sampleInterventions={plantLocationInfo.sampleInterventions} | ||
| setSelectedSamplePlantLocation={setSelectedSamplePlantLocation} | ||
| /> | ||
| ), | ||
| ].filter(Boolean); |
There was a problem hiding this comment.
Fix duplicate keys and improve conditional rendering.
The content array has several issues that need to be addressed:
- Duplicate key "plantingDetails"
- Complex conditional rendering that can be simplified
const content = [
<>
<InterventionHeader
plHid={plantLocationInfo?.hid}
interventionType={plantLocationInfo?.type}
plantDate={plantLocationInfo?.interventionStartDate}
key="interventionHeader"
/>
{shouldDisplayImageCarousel && (
<ImageSlider
key="imageSlider"
images={sampleInterventionSpeciesImages}
type="coordinate"
isMobile={isMobile}
imageSize="large"
allowFullView={!isMobile}
/>
)}
</>,
cleanedPublicMetadata.length > 0 && (
<OtherInterventionMetadata
- key="plantingDetails"
+ key="interventionMetadata"
metadata={cleanedPublicMetadata}
plantDate={plantLocationInfo?.interventionStartDate}
type={plantLocationInfo?.type}
/>),
plantLocationInfo?.plantedSpecies?.length > 0 && (
<SpeciesPlanted
key="speciesPlanted"
totalTreesCount={totalTreesCount}
plantedSpecies={plantLocationInfo.plantedSpecies}
/>
),
- plantLocationInfo &&
- plantLocationInfo.sampleInterventions &&
- plantLocationInfo.sampleInterventions.length > 0 && (
+ plantLocationInfo?.sampleInterventions?.length > 0 && (
<SampleTrees
key="sampleTrees"
sampleInterventions={plantLocationInfo.sampleInterventions}
setSelectedSamplePlantLocation={setSelectedSamplePlantLocation}
/>
),
].filter(Boolean);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const content = [ | |
| <> | |
| <InterventionHeader | |
| plHid={plantLocationInfo?.hid} | |
| interventionType={plantLocationInfo?.type} | |
| plantDate={plantLocationInfo?.interventionStartDate} | |
| key="interventionHeader" | |
| /> | |
| {shouldDisplayImageCarousel && ( | |
| <ImageSlider | |
| key="imageSlider" | |
| images={sampleInterventionSpeciesImages} | |
| type="coordinate" | |
| isMobile={isMobile} | |
| imageSize="large" | |
| allowFullView={!isMobile} | |
| /> | |
| )} | |
| </>, | |
| cleanedPublicMetadata.length > 0 && ( | |
| <OtherInterventionMetadata | |
| key="plantingDetails" | |
| metadata={cleanedPublicMetadata} | |
| plantDate={plantLocationInfo?.interventionStartDate} | |
| type={plantLocationInfo?.type} | |
| />), | |
| plantLocationInfo?.plantedSpecies && | |
| plantLocationInfo.plantedSpecies.length > 0 && ( | |
| <SpeciesPlanted | |
| key="speciesPlanted" | |
| totalTreesCount={totalTreesCount} | |
| plantedSpecies={plantLocationInfo.plantedSpecies} | |
| /> | |
| ), | |
| plantLocationInfo && | |
| plantLocationInfo.sampleInterventions && | |
| plantLocationInfo.sampleInterventions.length > 0 && ( | |
| <SampleTrees | |
| key="sampleTrees" | |
| sampleInterventions={plantLocationInfo.sampleInterventions} | |
| setSelectedSamplePlantLocation={setSelectedSamplePlantLocation} | |
| /> | |
| ), | |
| ].filter(Boolean); | |
| const content = [ | |
| <> | |
| <InterventionHeader | |
| plHid={plantLocationInfo?.hid} | |
| interventionType={plantLocationInfo?.type} | |
| plantDate={plantLocationInfo?.interventionStartDate} | |
| key="interventionHeader" | |
| /> | |
| {shouldDisplayImageCarousel && ( | |
| <ImageSlider | |
| key="imageSlider" | |
| images={sampleInterventionSpeciesImages} | |
| type="coordinate" | |
| isMobile={isMobile} | |
| imageSize="large" | |
| allowFullView={!isMobile} | |
| /> | |
| )} | |
| </>, | |
| cleanedPublicMetadata.length > 0 && ( | |
| <OtherInterventionMetadata | |
| key="interventionMetadata" | |
| metadata={cleanedPublicMetadata} | |
| plantDate={plantLocationInfo?.interventionStartDate} | |
| type={plantLocationInfo?.type} | |
| />), | |
| plantLocationInfo?.plantedSpecies?.length > 0 && ( | |
| <SpeciesPlanted | |
| key="speciesPlanted" | |
| totalTreesCount={totalTreesCount} | |
| plantedSpecies={plantLocationInfo.plantedSpecies} | |
| /> | |
| ), | |
| plantLocationInfo?.sampleInterventions?.length > 0 && ( | |
| <SampleTrees | |
| key="sampleTrees" | |
| sampleInterventions={plantLocationInfo.sampleInterventions} | |
| setSelectedSamplePlantLocation={setSelectedSamplePlantLocation} | |
| /> | |
| ), | |
| ].filter(Boolean); |
🧰 Tools
🪛 Biome (1.9.4)
[error] 185-186: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/features/projectsV2/ProjectsMap/InterventionDropDown/index.tsx (1)
65-82: Remove unnecessary fragment and inline styles.The code can be simplified by removing the unnecessary fragment and moving inline styles to CSS module.
- <> {interventionData && ( <div className={styles.labelTextContainer}> {isMobile ? ( <label className={styles.interventionsLabel}> {truncateString(tIntervention(interventionData?.value), 40)} </label> ) : ( <p className={styles.interventionName} - style={{ marginTop: '5px' }} + className={`${styles.interventionName} ${styles.interventionNameSpacing}`} > {truncateString(tIntervention(interventionData?.value), 40)} </p> )} </div> )} - </>Add to CSS module:
.interventionNameSpacing { margin-top: 5px; }Also applies to: 74-76
🧰 Tools
🪛 Biome (1.9.4)
[error] 65-82: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragment(lint/complexity/noUselessFragments)
src/utils/constants/intervention.ts (2)
4-20: Encapsulate color constants.The color constants should be encapsulated in an object for better maintainability.
-const SINGLE_TREE = '#007A49'; -const MULTI_TREE = '#007A49'; -const INVASIVE_SPECIES = '#EB5757'; -// ... other color constants +const interventionColors = { + SINGLE_TREE: '#007A49', + MULTI_TREE: '#007A49', + INVASIVE_SPECIES: '#EB5757', + FIRE_SUPPRESSION: '#F2C94C', + FIRE_PATROL: '#F2994A', + // ... other colors +} as const;
75-79: Remove unnecessary index property.The
indexproperty inAllInterventionsseems unnecessary as it's always sequential and can be derived from array index if needed.export const AllInterventions: Array<{ label: string; value: INTERVENTION_TYPE; - index: number; }> = [
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/features/projectsV2/ProjectDetails/components/microComponents/InterventionHeader.tsx(1 hunks)src/features/projectsV2/ProjectsMap/InterventionDropDown/InterventionList.tsx(1 hunks)src/features/projectsV2/ProjectsMap/InterventionDropDown/index.tsx(1 hunks)src/utils/constants/intervention.ts(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
src/features/projectsV2/ProjectsMap/InterventionDropDown/index.tsx
[error] 65-82: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragment
(lint/complexity/noUselessFragments)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (javascript)
🔇 Additional comments (4)
src/features/projectsV2/ProjectsMap/InterventionDropDown/InterventionList.tsx (1)
31-48: 🛠️ Refactor suggestionImprove accessibility and list rendering.
The list implementation needs several improvements:
- Using index as key in map function is an anti-pattern
- Missing accessibility attributes for interactive elements
- Class name
siteListOptionsshould be renamed to match intervention context- <ul className={styles.siteListOptions}> + <ul className={styles.interventionListOptions} role="listbox"> {interventionList.map((intervention, index) => { return ( <li className={`${styles.listItem} ${intervention.value === selectedInterventionData?.value ? styles.selectedItem : ''}`} onClick={() => handleFilterSelection(intervention.value)} - key={index} + key={intervention.value} + role="option" + aria-selected={intervention.value === selectedInterventionData?.value} + tabIndex={0} + onKeyDown={(e) => { + if (e.key === 'Enter' || e.key === ' ') { + handleFilterSelection(intervention.value); + } + }} > <p>{tProjectDetails(intervention.value)}</p> </li> ); })} </ul>Likely invalid or redundant comment.
src/features/projectsV2/ProjectsMap/InterventionDropDown/index.tsx (2)
48-52:⚠️ Potential issueFix the useEffect logic condition.
The menu should close when
disableInterventionMenuis true, not false.useEffect(() => { - if (!disableInterventionMenu) { + if (disableInterventionMenu) { setIsMenuOpen(false); } }, [disableInterventionMenu]);Likely invalid or redundant comment.
39-46: 🛠️ Refactor suggestionAdd error boundary for undefined allInterventions.
The
useMemohook handles undefinedallInterventions, but it would be better to validate the prop at component level.+ if (!allInterventions) { + console.warn('InterventionDropdown: allInterventions prop is required'); + return null; + } const interventionList = useMemo(() => { - if (!allInterventions) return []; return allInterventions.map((el) => ({ label: el.label, index: el.index, value: el.value })); }, [allInterventions]);Likely invalid or redundant comment.
src/utils/constants/intervention.ts (1)
135-137: 🛠️ Refactor suggestionImprove type safety.
The function should use
INTERVENTION_TYPEinstead ofstringfor better type safety.-export const findMatchingIntervention = (value: string) => { +export const findMatchingIntervention = (value: INTERVENTION_TYPE) => { return AllInterventions.find((item) => item.value === value); };Likely invalid or redundant comment.
| @@ -0,0 +1,36 @@ | |||
| import styles from '../../styles/PlantLocationInfo.module.scss'; | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Update class names and styles.
The component is reusing styles from PlantLocationInfo.module.scss and class names related to plant location. Create a new module for intervention-specific styles.
-import styles from '../../styles/PlantLocationInfo.module.scss';
+import styles from './InterventionHeader.module.scss';
- className={`plant-location-header-container ${styles.plantLocationHeaderContainer}`}
+ className={styles.interventionHeaderContainer}Also applies to: 20-21
mariahosfeld
dismissed
sunilsabatp’s stale review
All remarks should be resolved.
| const shouldShowOtherIntervention = | ||
| props.isMobile && | ||
| selectedPlantLocation !== null && | ||
| !PLANTATION_TYPES.includes(selectedPlantLocation.type); |
There was a problem hiding this comment.
This code can be optimized by using the existing helper function isNonPlantationType as following:
const shouldShowOtherIntervention = props.isMobile && isNonPlantationType(selectedPlantLocation);
Fixes #
Changes in this pull request:
Displaying other interventions in the web app.