-
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
Feature/intervention #2343
base: develop
Are you sure you want to change the base?
Feature/intervention #2343
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
53257dc
to
43d3856
Compare
5fb466c
to
9f9021c
Compare
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
♻️ Duplicate comments (3)
src/utils/constants/intervention.ts (3)
4-20
: 🛠️ Refactor suggestionEncapsulate color constants for better maintainability.
The color constants should be encapsulated in an object for better organization and 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', + // ... other colors +} as const; +type InterventionColor = typeof InterventionColors[keyof typeof InterventionColors];
48-88
: 🛠️ Refactor suggestionImprove AllInterventions configuration.
Several improvements needed:
- Labels should be translatable for internationalization
- The
index
property is redundant as array indices can be used if needed- Formatting is inconsistent
+import { useTranslation } from 'next-i18next'; + export const AllInterventions: Array<{ label: string value: INTERVENTION_TYPE - index: number }> = [ { - label: 'All Intervention', + label: t('interventions.all'), value: 'all', - index: 0, }, { - label: 'Single/Multi Tree Plantation', + label: t('interventions.default'), value: 'default', - index: 1, }, // ... format other entries consistently ]
98-100
: 🛠️ Refactor suggestionImprove type safety in findMatchingIntervention.
The function should use the INTERVENTION_TYPE instead of string for better type safety.
-export const findMatchingIntervention = (value: string) => { +export const findMatchingIntervention = (value: INTERVENTION_TYPE) => { return AllInterventions.find(item => item.value === value); };
🧹 Nitpick comments (4)
src/features/projectsV2/ProjectDetails/components/OtherInterventionInfo.tsx (2)
116-128
: Simplify totalTreesCount calculation using optional chaining.The current implementation has unnecessary complexity in the conditions and dependencies.
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] 118-119: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
129-142
: Add return type and simplify sampleInterventionSpeciesImages logic.The function lacks a return type annotation and has unnecessary complexity.
- 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] 132-139: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
src/utils/constants/intervention.ts (2)
45-45
: Improve type organization for better maintainability.The INTERVENTION_TYPE union type is quite long. Consider breaking it down into categorized sub-types for better organization and maintainability.
+type BaseInterventionType = 'unknown' | 'default' | 'all'; + +type PlantationType = 'single-tree-registration' | 'multi-tree-registration'; + +type FireRelatedType = 'fire-patrol' | 'fire-suppression' | 'firebreaks'; + +type MaintenanceType = + | 'removal-invasive-species' + | 'fencing' + | 'grass-suppression' + | 'maintenance'; + +type RegenerationType = + | 'marking-regenerant' + | 'liberating-regenerant' + | 'direct-seeding' + | 'enrichment-planting' + | 'assisting-seed-rain'; + +type OtherType = 'soil-improvement' | 'stop-tree-harvesting' | 'other-intervention'; + -export type INTERVENTION_TYPE = 'single-tree-registration' | 'multi-tree-registration' | ...; +export type INTERVENTION_TYPE = + | BaseInterventionType + | PlantationType + | FireRelatedType + | MaintenanceType + | RegenerationType + | OtherType;
103-106
: Enhance type safety and error handling in findInterventionHeader.The function should use INTERVENTION_TYPE and handle edge cases more explicitly.
-export const findInterventionHeader = (valueKey: string | undefined) => { +export const findInterventionHeader = (valueKey: INTERVENTION_TYPE | undefined): string => { const found = AllInterventions.find(item => item.value === valueKey); - return found ? found.label : ''; + return found?.label ?? t('interventions.unknown'); };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/features/projectsV2/ProjectDetails/components/OtherInterventionInfo.tsx
(1 hunks)src/features/projectsV2/ProjectDetails/components/microComponents/InterventionHeader.tsx
(1 hunks)src/features/projectsV2/ProjectDetails/components/microComponents/OtherInterventionMetaData.tsx
(1 hunks)src/features/projectsV2/ProjectDetails/styles/PlantLocationInfo.module.scss
(4 hunks)src/utils/constants/intervention.ts
(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/features/projectsV2/ProjectDetails/components/microComponents/OtherInterventionMetaData.tsx
- src/features/projectsV2/ProjectDetails/components/microComponents/InterventionHeader.tsx
- src/features/projectsV2/ProjectDetails/styles/PlantLocationInfo.module.scss
🧰 Additional context used
🪛 Biome (1.9.4)
src/features/projectsV2/ProjectDetails/components/OtherInterventionInfo.tsx
[error] 118-119: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 132-139: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 192-193: 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 (2)
src/features/projectsV2/ProjectDetails/components/OtherInterventionInfo.tsx (1)
217-217
: LGTM!The default export follows React component conventions.
src/utils/constants/intervention.ts (1)
22-44
: Review the default fallback color in FillColor mapping.The FillColor mapping uses 'SINGLE_TREE' as the default fallback color. Consider if this is the most appropriate default or if a neutral color would be better for unknown types.
Also, consider using the encapsulated colors:
export const FillColor: DataDrivenPropertyValueSpecification<string> = [ 'match', ['get', 'type'], 'remeasurement', 'tomato', - 'single-tree-registration', SINGLE_TREE, + 'single-tree-registration', InterventionColors.SINGLE_TREE, // ... other mappings - SINGLE_TREE + InterventionColors.OTHER_INTERVENTION // neutral fallback color ]
const content = [ | ||
<> | ||
<InterventionHeader | ||
plHid={plantLocationInfo?.hid} | ||
interventionType={plantLocationInfo?.type} | ||
key="interventionHeader" | ||
/> | ||
{shouldDisplayImageCarousel && ( | ||
<ImageSlider | ||
key="imageSlider" | ||
images={sampleInterventionSpeciesImages} | ||
type="coordinate" | ||
isMobile={isMobile} | ||
imageSize="large" | ||
allowFullView={!isMobile} | ||
/> | ||
)} | ||
</>, | ||
|
||
cleanedPublicMetadata.length > 0 && ( | ||
<> | ||
<OtherInterventionInfoHeader | ||
key="interventionHeader" | ||
plantDate={plantLocationInfo?.interventionStartDate} | ||
/> | ||
<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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix duplicate keys and improve conditional rendering in content array.
There are several issues in the content array that need to be addressed:
- Duplicate key "interventionHeader"
- Complex conditional rendering
- Unnecessary comma after OtherInterventionMetadata
const content = [
<>
<InterventionHeader
plHid={plantLocationInfo?.hid}
interventionType={plantLocationInfo?.type}
key="interventionHeader"
/>
{shouldDisplayImageCarousel && (
<ImageSlider
key="imageSlider"
images={sampleInterventionSpeciesImages}
type="coordinate"
isMobile={isMobile}
imageSize="large"
allowFullView={!isMobile}
/>
)}
</>,
cleanedPublicMetadata.length > 0 && (
<>
<OtherInterventionInfoHeader
- key="interventionHeader"
+ key="otherInterventionHeader"
plantDate={plantLocationInfo?.interventionStartDate}
/>
<OtherInterventionMetadata
key="plantingDetails"
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} | |
key="interventionHeader" | |
/> | |
{shouldDisplayImageCarousel && ( | |
<ImageSlider | |
key="imageSlider" | |
images={sampleInterventionSpeciesImages} | |
type="coordinate" | |
isMobile={isMobile} | |
imageSize="large" | |
allowFullView={!isMobile} | |
/> | |
)} | |
</>, | |
cleanedPublicMetadata.length > 0 && ( | |
<> | |
<OtherInterventionInfoHeader | |
key="interventionHeader" | |
plantDate={plantLocationInfo?.interventionStartDate} | |
/> | |
<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} | |
key="interventionHeader" | |
/> | |
{shouldDisplayImageCarousel && ( | |
<ImageSlider | |
key="imageSlider" | |
images={sampleInterventionSpeciesImages} | |
type="coordinate" | |
isMobile={isMobile} | |
imageSize="large" | |
allowFullView={!isMobile} | |
/> | |
)} | |
</>, | |
cleanedPublicMetadata.length > 0 && ( | |
<> | |
<OtherInterventionInfoHeader | |
key="otherInterventionHeader" | |
plantDate={plantLocationInfo?.interventionStartDate} | |
/> | |
<OtherInterventionMetadata | |
key="plantingDetails" | |
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] 192-193: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
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: 2
🧹 Nitpick comments (1)
src/features/projectsV2/ProjectDetails/components/microComponents/OtherInterventionMetadata.tsx (1)
10-10
: Fix spacing in condition check.Maintain consistent spacing around operators.
- if(metadata.length===0){ + if (metadata.length === 0) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/features/projectsV2/ProjectDetails/components/microComponents/OtherInterventionMetadata.tsx
(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (javascript)
🔇 Additional comments (2)
src/features/projectsV2/ProjectDetails/components/microComponents/OtherInterventionMetadata.tsx (2)
30-30
: LGTM!The default export is appropriate for this component.
14-14
: Verify styling implementation.Based on PR feedback about container gaps and dropdown heights, please verify that the styles applied here (
interventionMetadataGroup
andinterventionMetaItem
) properly handle:
- Container spacing
- Height constraints
- Mobile responsiveness
Also applies to: 19-19
✅ Verification successful
Styling implementation is appropriate ✅
The current implementation uses a flexible column layout with consistent spacing and padding. The container gaps are properly handled through margins and the flex layout ensures proper content flow. No problematic height constraints are present that could affect the dropdown behavior.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for responsive styling and container gap definitions rg -l "interventionMetadataGroup|interventionMetaItem" src/features/projectsV2/ProjectDetails/styles/ # Look for media queries and container styling rg "(@media|gap|height|max-height)" src/features/projectsV2/ProjectDetails/styles/PlantLocationInfo.module.scssLength of output: 398
Script:
#!/bin/bash # Get the full content of the stylesheet to see class definitions cat src/features/projectsV2/ProjectDetails/styles/PlantLocationInfo.module.scss # Check for any responsive styles in parent components rg -g '*.scss' -g '*.css' "@media.*{" src/features/projectsV2/ProjectDetails/Length of output: 3795
interface Props { | ||
metadata: { key: string; value: string }[]; | ||
type: string | undefined, | ||
plantDate: string | null | undefined; | ||
} |
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.
🛠️ Refactor suggestion
Remove unused props and enhance type safety.
The Props interface includes unused properties (type
and plantDate
) and could benefit from stricter typing:
interface Props {
- metadata: { key: string; value: string }[];
- type: string | undefined,
- plantDate: string | null | undefined;
+ /** Array of metadata key-value pairs to display */
+ metadata: ReadonlyArray<{
+ readonly key: string;
+ readonly value: string;
+ }>;
}
📝 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.
interface Props { | |
metadata: { key: string; value: string }[]; | |
type: string | undefined, | |
plantDate: string | null | undefined; | |
} | |
interface Props { | |
/** Array of metadata key-value pairs to display */ | |
metadata: ReadonlyArray<{ | |
readonly key: string; | |
readonly value: string; | |
}>; | |
} |
{metadata.map((item, key) => { | ||
return ( | ||
<div | ||
key={key} | ||
className={`planting-details-item ${styles.interventionMetaItem}`} | ||
> | ||
<h2 className={styles.label}>{item.key}</h2> | ||
<p className={styles.data}>{item.value}</p> | ||
</div> | ||
); | ||
})} |
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.
🛠️ Refactor suggestion
Improve accessibility and avoid using array indices as keys.
The current implementation has accessibility issues and uses array indices as React keys, which is not recommended:
- Add semantic HTML structure with proper ARIA labels
- Use unique identifiers instead of array indices as keys
- {metadata.map((item, key) => {
+ {metadata.map((item) => {
return (
<div
- key={key}
+ key={`${item.key}-${item.value}`}
className={`planting-details-item ${styles.interventionMetaItem}`}
+ role="group"
+ aria-label={`${item.key} details`}
>
- <h2 className={styles.label}>{item.key}</h2>
- <p className={styles.data}>{item.value}</p>
+ <h2 className={styles.label} id={`label-${item.key}`}>{item.key}</h2>
+ <p className={styles.data} aria-labelledby={`label-${item.key}`}>{item.value}</p>
</div>
);
})}
📝 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.
{metadata.map((item, key) => { | |
return ( | |
<div | |
key={key} | |
className={`planting-details-item ${styles.interventionMetaItem}`} | |
> | |
<h2 className={styles.label}>{item.key}</h2> | |
<p className={styles.data}>{item.value}</p> | |
</div> | |
); | |
})} | |
{metadata.map((item) => { | |
return ( | |
<div | |
key={`${item.key}-${item.value}`} | |
className={`planting-details-item ${styles.interventionMetaItem}`} | |
role="group" | |
aria-label={`${item.key} details`} | |
> | |
<h2 className={styles.label} id={`label-${item.key}`}>{item.key}</h2> | |
<p className={styles.data} aria-labelledby={`label-${item.key}`}>{item.value}</p> | |
</div> | |
); | |
})} |
src/features/projectsV2/ProjectDetails/components/OtherInterventionInfo.tsx
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.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/features/projectsV2/ProjectDetails/components/OtherInterventionInfo.tsx
(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
src/features/projectsV2/ProjectDetails/components/OtherInterventionInfo.tsx
[error] 118-119: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 132-139: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 190-191: 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/OtherInterventionInfo.tsx (1)
28-35
: LGTM! Well-implemented JSON validation.The function includes proper error handling and type checking.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Move createCardData
to a dedicated utility file.
The function is well-implemented with good documentation, but for better maintainability and reusability, it should be moved to a separate utility file.
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...
}
const sampleInterventionSpeciesImages = useMemo(() => { | ||
if (plantLocationInfo && plantLocationInfo.sampleInterventions.length > 0) { | ||
const result = | ||
plantLocationInfo.sampleInterventions && | ||
plantLocationInfo.sampleInterventions.map((item) => { | ||
return { | ||
id: item.coordinates[0].id, | ||
image: item.coordinates[0].image ?? '', | ||
description: tProjectDetails('sampleTreeTag', { tag: item.tag }), | ||
}; | ||
}); | ||
return result; | ||
} | ||
}, [plantLocationInfo]); |
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.
🛠️ Refactor suggestion
Simplify sampleInterventionSpeciesImages logic.
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]);
📝 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 sampleInterventionSpeciesImages = useMemo(() => { | |
if (plantLocationInfo && plantLocationInfo.sampleInterventions.length > 0) { | |
const result = | |
plantLocationInfo.sampleInterventions && | |
plantLocationInfo.sampleInterventions.map((item) => { | |
return { | |
id: item.coordinates[0].id, | |
image: item.coordinates[0].image ?? '', | |
description: tProjectDetails('sampleTreeTag', { tag: item.tag }), | |
}; | |
}); | |
return result; | |
} | |
}, [plantLocationInfo]); | |
const sampleInterventionSpeciesImages = useMemo(() => { | |
if (plantLocationInfo?.sampleInterventions?.length > 0) { | |
return plantLocationInfo.sampleInterventions.map((item) => { | |
return { | |
id: item.coordinates[0].id, | |
image: item.coordinates[0]?.image ?? '', | |
description: tProjectDetails('sampleTreeTag', { tag: item.tag }), | |
}; | |
}); | |
} | |
return []; | |
}, [plantLocationInfo]); |
🧰 Tools
🪛 Biome (1.9.4)
[error] 132-139: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
const { totalTreesCount } = useMemo(() => { | ||
const totalTreesCount = | ||
plantLocationInfo && | ||
plantLocationInfo.plantedSpecies && | ||
plantLocationInfo.plantedSpecies.length > 0 | ||
? plantLocationInfo.plantedSpecies.reduce( | ||
(sum, species: { treeCount: number }) => sum + species.treeCount, | ||
0 | ||
) | ||
: 0; | ||
return { totalTreesCount }; | ||
}, [plantLocationInfo, plantLocationInfo?.type]); | ||
|
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.
🛠️ Refactor suggestion
Optimize useMemo implementation.
- Simplify the conditional check using optional chaining
- 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]);
📝 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 { totalTreesCount } = useMemo(() => { | |
const totalTreesCount = | |
plantLocationInfo && | |
plantLocationInfo.plantedSpecies && | |
plantLocationInfo.plantedSpecies.length > 0 | |
? plantLocationInfo.plantedSpecies.reduce( | |
(sum, species: { treeCount: number }) => sum + species.treeCount, | |
0 | |
) | |
: 0; | |
return { totalTreesCount }; | |
}, [plantLocationInfo, plantLocationInfo?.type]); | |
const { totalTreesCount } = useMemo(() => { | |
const totalTreesCount = | |
plantLocationInfo?.plantedSpecies?.length > 0 | |
? plantLocationInfo.plantedSpecies.reduce( | |
(sum, species: { treeCount: number }) => sum + species.treeCount, | |
0 | |
) | |
: 0; | |
return { totalTreesCount }; | |
}, [plantLocationInfo?.plantedSpecies]); |
🧰 Tools
🪛 Biome (1.9.4)
[error] 118-119: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
const content = [ | ||
<> | ||
<InterventionHeader | ||
plHid={plantLocationInfo?.hid} | ||
interventionType={plantLocationInfo?.type} | ||
key="interventionHeader" | ||
/> | ||
{shouldDisplayImageCarousel && ( | ||
<ImageSlider | ||
key="imageSlider" | ||
images={sampleInterventionSpeciesImages} | ||
type="coordinate" | ||
isMobile={isMobile} | ||
imageSize="large" | ||
allowFullView={!isMobile} | ||
/> | ||
)} | ||
</>, | ||
cleanedPublicMetadata.length > 0 && ( | ||
<> | ||
<OtherInterventionInfoHeader | ||
key="interventionHeader" | ||
plantDate={plantLocationInfo?.interventionStartDate} | ||
/> | ||
<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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix duplicate keys and improve conditional rendering.
- Duplicate key "interventionHeader" between components
- Complex conditional rendering that could be simplified
const content = [
<>
<InterventionHeader
plHid={plantLocationInfo?.hid}
interventionType={plantLocationInfo?.type}
key="interventionHeader"
/>
{shouldDisplayImageCarousel && (
<ImageSlider
key="imageSlider"
images={sampleInterventionSpeciesImages}
type="coordinate"
isMobile={isMobile}
imageSize="large"
allowFullView={!isMobile}
/>
)}
</>,
cleanedPublicMetadata.length > 0 && (
<>
<OtherInterventionInfoHeader
- key="interventionHeader"
+ key="otherInterventionHeader"
plantDate={plantLocationInfo?.interventionStartDate}
/>
<OtherInterventionMetadata
key="plantingDetails"
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} | |
key="interventionHeader" | |
/> | |
{shouldDisplayImageCarousel && ( | |
<ImageSlider | |
key="imageSlider" | |
images={sampleInterventionSpeciesImages} | |
type="coordinate" | |
isMobile={isMobile} | |
imageSize="large" | |
allowFullView={!isMobile} | |
/> | |
)} | |
</>, | |
cleanedPublicMetadata.length > 0 && ( | |
<> | |
<OtherInterventionInfoHeader | |
key="interventionHeader" | |
plantDate={plantLocationInfo?.interventionStartDate} | |
/> | |
<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} | |
key="interventionHeader" | |
/> | |
{shouldDisplayImageCarousel && ( | |
<ImageSlider | |
key="imageSlider" | |
images={sampleInterventionSpeciesImages} | |
type="coordinate" | |
isMobile={isMobile} | |
imageSize="large" | |
allowFullView={!isMobile} | |
/> | |
)} | |
</>, | |
cleanedPublicMetadata.length > 0 && ( | |
<> | |
<OtherInterventionInfoHeader | |
key="otherInterventionHeader" | |
plantDate={plantLocationInfo?.interventionStartDate} | |
/> | |
<OtherInterventionMetadata | |
key="plantingDetails" | |
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] 190-191: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
…n constants - also adds comments to explain FillColor config
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
♻️ Duplicate comments (6)
src/utils/constants/intervention.ts (3)
4-52
: 🛠️ Refactor suggestionImprove color organization and consistency.
The color constants need better organization and consistency:
- Encapsulate colors in an object for better maintainability
- Use a consistent color format (hex instead of named colors)
- Consider using CSS custom properties for theming
-const SINGLE_TREE = '#007A49'; -const MULTI_TREE = '#007A49'; -// ... other color constants +export const interventionColors = { + SINGLE_TREE: '#007A49', + MULTI_TREE: '#007A49', + INVASIVE_SPECIES: '#EB5757', + // ... other colors +} as const; export const FillColor: DataDrivenPropertyValueSpecification<string> = [ 'match', ['get', 'type'], - 'remeasurement', 'tomato', + 'remeasurement', '#FF6347', // Using hex instead of 'tomato' 'single-tree-registration', interventionColors.SINGLE_TREE, // ... other mappings ];
76-120
: 🛠️ Refactor suggestionImprove AllInterventions configuration.
Several improvements needed:
- Labels should be translatable for internationalization
- The
index
property is redundant as it's always sequential- Standardize label formatting
+import { useTranslation } from 'next-i18next'; + export const AllInterventions: Array<{ label: string; value: INTERVENTION_TYPE; - index: number; }> = [ { - label: 'All Intervention', + label: t('interventions.all'), value: 'all', - index: 0, }, { - label: 'Single/Multi Tree Plantation', + label: t('interventions.default'), value: 'default', - index: 1, }, // ... other interventions ];
136-143
: 🛠️ Refactor suggestionImprove type safety and error handling.
The utility functions need better type safety:
- Use
INTERVENTION_TYPE
instead ofstring
- Make return types explicit
- Handle edge cases better
-export const findMatchingIntervention = (value: string) => { +export const findMatchingIntervention = ( + value: INTERVENTION_TYPE +): { label: string; value: INTERVENTION_TYPE } | null => { return AllInterventions.find((item) => item.value === value) ?? null; }; -export const findInterventionHeader = (valueKey: string | undefined) => { +export const findInterventionHeader = ( + valueKey: INTERVENTION_TYPE | undefined +): string => { const found = AllInterventions.find((item) => item.value === valueKey); return found ? found.label : ''; };src/features/projectsV2/ProjectsMap/InterventionDropDown/index.tsx (3)
45-49
:⚠️ Potential issueFix the useEffect logic condition.
The menu should close when
disableInterventionMenu
is true, not false.useEffect(() => { - if (!disableInterventionMenu) { + if (disableInterventionMenu) { setIsMenuOpen(false); } }, [disableInterventionMenu]);
58-88
: 🛠️ Refactor suggestionImprove accessibility and component structure.
- Remove unnecessary fragments
- Add proper ARIA attributes
- Move inline styles to CSS
- Fix naming inconsistencies (remove references to "site")
- <> <div className={styles.dropdownButton} onClick={toggleMenu} + role="button" + aria-haspopup="listbox" + aria-expanded={isMenuOpen} + tabIndex={0} + onKeyDown={(e) => { + if (e.key === 'Enter' || e.key === ' ') { + toggleMenu(); + } + }} > - <div className={styles.siteIconAndTextContainer}> + <div className={styles.interventionIconAndTextContainer}> <InterventionIcon /> - <> {interventionData && ( <div className={styles.labelTextContainer}> {isMobile ? ( - <label className={styles.sitesLabel}> + <label className={styles.interventionsLabel}> {truncateString(interventionData?.label, 40)} </label> ) : ( <p - className={styles.siteName} - style={{ marginTop: '5px' }} + className={`${styles.interventionName} ${styles.interventionNameSpacing}`} > {truncateString(interventionData?.label, 40)} </p> )} </div> )} - </>Add to CSS module:
.interventionNameSpacing { margin-top: 5px; }🧰 Tools
🪛 Biome (1.9.4)
[error] 62-79: 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)
89-96
: 🛠️ Refactor suggestionAddress dropdown height and mobile view issues.
Per PR objectives, the dropdown height needs adjustment to prevent overlapping and improve mobile view.
Add to CSS module:
.interventionList { max-height: 300px; overflow-y: auto; @media (max-width: 768px) { max-height: 250px; position: absolute; z-index: 1; } }
🧹 Nitpick comments (2)
src/utils/constants/intervention.ts (1)
54-74
: Add documentation and consider using const objects for type safety.The type definition could be improved:
- Add JSDoc documentation explaining the purpose and usage
- Consider generating the type from a const object to ensure consistency with FillColor mapping
- Document special types like 'unknown', 'default', and 'all'
+/** + * Represents all possible intervention types in the system. + * Special types: + * - 'unknown': Used when the intervention type cannot be determined + * - 'default': Represents both single and multi-tree plantations + * - 'all': Used in filters to show all interventions + */ export type INTERVENTION_TYPE = | 'single-tree-registration' // ... other typessrc/features/projectsV2/ProjectsMap/InterventionDropDown/index.tsx (1)
1-16
: Clean up type definitions and imports.
- Move the
InterventionOptionType
interface to a separate types file for better organization.- Consider using path aliases to simplify import paths.
-import type { SetState } from '../../../common/types/common'; -import type { INTERVENTION_TYPE } from '../../../../utils/constants/intervention'; +import type { SetState } from '@/common/types/common'; +import type { INTERVENTION_TYPE } from '@/utils/constants/intervention'; -interface InterventionOptionType { - label: string; - value: INTERVENTION_TYPE; - index: number; -}Create a new file
src/features/projectsV2/types/intervention.ts
:export interface InterventionOptionType { 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/ProjectsContext.tsx
(6 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)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/features/projectsV2/ProjectsContext.tsx
- src/features/projectsV2/ProjectsMap/InterventionDropDown/InterventionList.tsx
🧰 Additional context used
🪛 Biome (1.9.4)
src/features/projectsV2/ProjectsMap/InterventionDropDown/index.tsx
[error] 62-79: 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 (2)
src/features/projectsV2/ProjectsMap/InterventionDropDown/index.tsx (2)
10-10
: Move utility function to utils file.The
findMatchingIntervention
function should be moved to a utils file.
35-43
: 🛠️ Refactor suggestionSimplify useMemo and add prop validation.
The
useMemo
hook can be simplified, and we should add validation for required props.+ if (!allInterventions) { + console.warn('InterventionDropdown: allInterventions prop is required'); + return null; + } + const [isMenuOpen, setIsMenuOpen] = useState(false); const interventionList = useMemo(() => { - if (!allInterventions) return []; - return allInterventions.map((el) => ({ + return allInterventions?.map((el) => ({ label: el.label, index: el.index, value: el.value, })); }, [allInterventions]);Likely invalid or redundant comment.
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 Added 2 points (1 minor point and one that should be addressed). Once done, LGTM.
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: 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
disableInterventionMenu
This 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
.interventionTitle
class 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 betweenOtherInterventions
andPlantLocationMulti
.The
OtherInterventions
interface 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
PlantLocationBase
types 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: 10px
improves 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 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-height
for 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.
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 (3)
src/features/projectsV2/ProjectDetails/components/OtherInterventionInfo.tsx (3)
36-100
: MovecreateCardData
to 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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Fix duplicate keys and improve conditional rendering in content array.
There are several issues in the content array:
- Duplicate key "plantingDetails"
- Complex conditional rendering that could 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] 183-184: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
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
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.
When hovering over a different intervention location than tree plantings, the empty box for intervention data is shown. Compare to any tree plant location and adapt behavior to already show data on hover.
Fixes #
Changes in this pull request:
Displaying other interventions in the web app.