Post Launch feedback for projects facelift #2302
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
WalkthroughThe pull request introduces several changes across multiple files, including updates to styling in SCSS files, modifications to React components for improved state management, and enhancements to a date formatting utility. Key changes involve the transition from prop-based state management to context-based management for map-related components, as well as adjustments to layout properties and conditional rendering logic. Additionally, the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
mohitb35
force-pushed
the
projects-redesign/launch-feedback
branch
from
5c194f0 to
751c733
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (16)
src/utils/mapsV2/zoomToLocation.tsx (1)
35-35: Consider adding error handling for the state update.While the implementation is correct, it might be good to wrap the state update in a try-catch block since it occurs in an event callback. This would help catch and handle any potential errors that might occur during state updates.
- handleViewStateChange(newViewState); + try { + handleViewStateChange(newViewState); + } catch (error) { + console.error('Failed to update view state:', error); + }src/utils/countryCurrency/getFormattedDate.ts (2)
24-26: Consider extracting locale configuration.The locale configuration is duplicated between string and non-string cases.
Extract the common configuration:
+const getLocaleConfig = () => ({ + locale: localeMapForDate[localStorage.getItem('language') || 'en'], +}); export default function formatDate(date: number | Date | string) { // ... existing code ... return format(parseISO(isoDateString), 'LLLL d, yyyy', { - locale: localeMapForDate[localStorage.getItem('language') || 'en'], + ...getLocaleConfig(), }); // ... existing code ... return format(date, 'LLLL d, yyyy', { - locale: localeMapForDate[localStorage.getItem('language') || 'en'], + ...getLocaleConfig(), });
5-5: Add JSDoc documentation for the function.Document the function's behavior, especially:
- Expected date string format
- Return value in different scenarios (falsy input, invalid format, error)
- Locale handling
Add documentation:
+/** + * Formats a date into a localized string (e.g., "January 1, 2024") + * @param date - Date to format (ISO string YYYY-MM-DD, timestamp, or Date object) + * @returns Formatted date string or empty string if invalid/error + */ export default function formatDate(date: number | Date | string) {src/utils/mapsV2/zoomToPolygonPlantLocation.tsx (1)
Line range hint
12-47: Consider adding input validation.The function could benefit from additional validation to handle edge cases:
- Empty coordinates array
- Invalid polygon coordinates that might cause turf.js errors
Consider adding these checks:
export function zoomToPolygonPlantLocation( coordinates: Position[], mapRef: MapRef, handleViewStateChange: (viewState: Partial<ViewState>) => void, duration = 3000 ) { if (!mapRef.current) { return; } + if (!coordinates || coordinates.length < 3) { + console.warn('Invalid polygon: At least 3 coordinates required'); + return; + } + try { const polygon = turf.polygon([coordinates]); const bbox = turf.bbox(polygon); // ... rest of the implementation + } catch (error) { + console.error('Failed to process polygon coordinates:', error); + return; + } }src/utils/mapsV2/zoomToProjectSite.ts (1)
1-4: Rename Map type import to avoid shadowing global Map.To prevent confusion with the global
Maptype, consider using a more specific name for the maplibre-gl Map type.-import type { Map } from 'maplibre-gl'; +import type { Map as MapLibreMap } from 'maplibre-gl';🧰 Tools
🪛 Biome
[error] 3-3: Do not shadow the global "Map" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
src/features/projectsV2/ProjectsMap/ProjectsMap.module.scss (1)
63-81: LGTM! Consider adding a comment explaining the !important flags.The changes correctly implement container-based map sizing, ensuring the map properly fills its container. The use of
position: relativeandoverflow: hiddenprovides proper positioning context and prevents unwanted scrollbars.Consider adding a brief comment explaining why
!importantflags are necessary (to override maplibregl's inline styles).src/features/projectsV2/ProjectsMap/MultipleProjectsView.tsx (2)
7-7: Good architectural improvement moving to context-based state management!The transition from prop-based to context-based state management for map view state is a positive change that:
- Reduces prop drilling
- Centralizes map state management
- Makes the component more focused on its core responsibilities
Also applies to: 11-14
Line range hint
39-64: Optimize useMemo dependencies.The memoization implementation is good, but the dependency array includes unnecessary items.
isLoadingandisErrordon't affect the categorization logic and can be removed from the dependencies.}, [projects, filteredProjects, isLoading, isError]); + }, [projects, filteredProjects]);src/features/projectsV2/ProjectsMap/ProjectSiteDropDown/SiteDropdown.module.scss (1)
3-4: LGTM! Consider adding documentation for layout variables.The introduction of layout padding variables improves maintainability. Consider adding a brief comment explaining their purpose and relationship with other layout components.
+// Base padding values for project layout components $layoutPaddingTop: 24px; $layoutPaddingSide: 20px;src/features/projectsV2/ProjectsMapContext.tsx (2)
29-37: Consider adding runtime validation for width/height values.While setting default dimensions to '100%' is good, consider adding runtime validation to ensure these critical dimensions are never undefined or invalid, which could break the map rendering.
const validateViewState = (state: ExtendedViewState): ExtendedViewState => { return { ...state, width: state.width ?? '100%', height: state.height ?? '100%' }; };
107-111: Consider enhancing type safety for context value.While the context structure is clean, consider using a non-null assertion or proper type guard to ensure type safety of the context value.
const value = useMemo( (): ProjectsMapState => ({ mapState, viewState, handleViewStateChange, isSatelliteView, setIsSatelliteView, mapOptions, updateMapOption, }), [mapState, viewState, mapOptions, isSatelliteView] );src/features/projectsV2/ProjectsMap/SingleProjectView.tsx (3)
51-57: Consider optimizing zoom duration for better UX.While the function calls are correctly updated, the zoom duration of 4000ms (4 seconds) might feel slow to users. Consider reducing this value to 2000-2500ms for a snappier experience while still maintaining smooth animation.
- handleViewStateChange, - 4000 + handleViewStateChange, + 2500
70-71: Maintain consistent zoom duration across the component.For consistency with the earlier suggestion, consider updating this zoom duration as well.
handleViewStateChange, - 4000 + 2500
Line range hint
39-60: Add cleanup functions to useEffect hooks.The zoom effects should clean up their animations when the component unmounts or dependencies change. This prevents potential race conditions with concurrent animations.
useEffect(() => { + let isActive = true; if (!router.isReady || selectedPlantLocation === null) return; // ... existing zoom logic ... + + return () => { + isActive = false; + mapRef.current?.stop(); // Stop any ongoing animations + }; }, [selectedPlantLocation, router.isReady]); useEffect(() => { + let isActive = true; if (!router.isReady || selectedPlantLocation !== null) return; // ... existing zoom logic ... + + return () => { + isActive = false; + mapRef.current?.stop(); // Stop any ongoing animations + }; }, [selectedSite, sitesGeojson, router.isReady, selectedPlantLocation]);Also applies to: 63-87
src/features/projectsV2/ProjectDetails/components/KeyInfo.tsx (1)
52-114: Clean and well-organized conditional rendering!The restructured conditional blocks effectively group related information and leverage the visibility flags for cleaner JSX. The implementation successfully simplifies the conditional rendering as intended.
Consider extracting the repeated
styles.keyInfoSubContainerdiv pattern into a reusable component for even better maintainability.+const KeyInfoSection: React.FC<{ show: boolean; children: React.ReactNode }> = ({ + show, + children, +}) => { + if (!show) return null; + return <div className={styles.keyInfoSubContainer}>{children}</div>; +}; const KeyInfo = ({...}) => { // ... return ( <div className={styles.keyInfoContainer}> - {(showAbandonmentInfo || - restorationDate.length > 0 || - showProtectionStarted) && ( - <div className={styles.keyInfoSubContainer}> + <KeyInfoSection + show={ + showAbandonmentInfo || + restorationDate.length > 0 || + showProtectionStarted + } + > {/* ... */} - </div> - )} + </KeyInfoSection>src/features/projectsV2/ProjectsMap/index.tsx (1)
57-76: Consider extracting magic number as a named constantThe debounced map centering logic with the 30 projects threshold is a good solution to prevent unnecessary re-centering. However, the magic number
30should be extracted as a named constant for better maintainability.+const MAX_PROJECTS_FOR_AUTO_CENTER = 30; + useDebouncedEffect( () => { const shouldCenterMap = filteredProjects !== undefined && filteredProjects.length > 0 && - filteredProjects.length < 30 && + filteredProjects.length < MAX_PROJECTS_FOR_AUTO_CENTER && mapRef.current;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
package.jsonis excluded by!**/*.json
📒 Files selected for processing (14)
.gitignore(1 hunks)src/features/common/Layout/ProjectsLayout/ProjectsLayout.module.scss(1 hunks)src/features/projectsV2/ProjectDetails/ProjectDetails.module.scss(1 hunks)src/features/projectsV2/ProjectDetails/components/KeyInfo.tsx(1 hunks)src/features/projectsV2/ProjectsMap/MultipleProjectsView.tsx(3 hunks)src/features/projectsV2/ProjectsMap/ProjectSiteDropDown/SiteDropdown.module.scss(2 hunks)src/features/projectsV2/ProjectsMap/ProjectsMap.module.scss(1 hunks)src/features/projectsV2/ProjectsMap/SingleProjectView.tsx(3 hunks)src/features/projectsV2/ProjectsMap/index.tsx(6 hunks)src/features/projectsV2/ProjectsMapContext.tsx(5 hunks)src/utils/countryCurrency/getFormattedDate.ts(1 hunks)src/utils/mapsV2/zoomToLocation.tsx(2 hunks)src/utils/mapsV2/zoomToPolygonPlantLocation.tsx(2 hunks)src/utils/mapsV2/zoomToProjectSite.ts(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- .gitignore
🧰 Additional context used
🪛 Biome
src/utils/mapsV2/zoomToProjectSite.ts
[error] 3-3: Do not shadow the global "Map" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
🔇 Additional comments (24)
src/features/projectsV2/ProjectDetails/ProjectDetails.module.scss (1)
5-6: LGTM! Improved content visibility with better spacing.
The height calculation and padding changes work together effectively to:
- Extend the container height by 30px
- Add matching bottom padding to prevent content from touching the container edge
- Maintain scrollability while improving content visibility
This aligns well with the PR objective to allow project details to overflow to the bottom of the page.
src/utils/mapsV2/zoomToLocation.tsx (2)
1-2: LGTM! Type imports and parameter signature changes improve type safety.
The changes to use explicit type imports and the more flexible Partial<ViewState> parameter type are good improvements. This aligns with TypeScript best practices and allows for more granular state updates.
Also applies to: 5-5
Line range hint 1-37: Verify the impact on map re-centering behavior.
Given the PR objectives mention fixing map re-centering issues with filtered projects, let's verify how this function interacts with the project filtering logic.
✅ Verification successful
Let me try a different search strategy to understand the usage of this function and its interaction with filtering logic.
Let me try one more search with modified file extensions to get the actual usage context:
Based on the search results, I can now provide a final response:
Map re-centering behavior is properly integrated with project filtering
The code shows that the zoomToLocation function is correctly integrated with the project filtering system:
- In
ProjectsMap/index.tsx, there's a dedicated logic to handle map centering when filtered projects change:const shouldCenterMap = filteredProjects !== undefined && filteredProjects.length > 0 && filteredProjects.length < 30;
- The function properly updates the view state through
handleViewStateChangecallback, which is consistently used across both single and multiple project views - The implementation includes proper duration and zoom level controls for smooth transitions
The changes to zoomToLocation function maintain compatibility with both filtered and unfiltered project views, ensuring consistent map behavior.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for references to zoomToLocation in filter-related code
echo "Searching for filter-related code that uses zoomToLocation..."
rg -l "zoomToLocation.*filter" --type ts --type tsx
# Search for any state management code related to map centering
echo "Searching for map center state management..."
ast-grep --pattern 'const [$_] = useState<$_>({ latitude: $_, longitude: $_ })'
Length of output: 395
Script:
#!/bin/bash
# Search for all usages of zoomToLocation function
echo "Searching for all imports and usages of zoomToLocation..."
rg "zoomToLocation" -A 3 --type ts --type typescript --type tsx
# Search for files containing both map filtering and viewport/view state changes
echo "Searching for files with map filtering and view state..."
rg "filter.*projects?" --type ts --type typescript --type tsx -l | xargs rg "ViewState|viewState|handleViewStateChange" -l
# Look for map ref usage with filtering
echo "Searching for mapRef usage with filtering..."
rg "mapRef.*filter|filter.*mapRef" -A 3 --type ts --type typescript --type tsx
Length of output: 1553
Script:
#!/bin/bash
# Search for direct usages of zoomToLocation
echo "Searching for zoomToLocation usage..."
rg "zoomToLocation" -B 2 -A 5 src/
# Look for the filtering logic in map components
echo "Searching for filtering logic in ProjectsMap components..."
rg "filter" -B 2 -A 5 src/features/projectsV2/ProjectsMap/
# Check for viewState handling in map components
echo "Checking viewState handling..."
rg "handleViewStateChange" -B 2 -A 5 src/features/projectsV2/ProjectsMap/
Length of output: 23808
src/utils/countryCurrency/getFormattedDate.ts (1)
6-8: LGTM! Good defensive programming practice.
The early return for falsy values prevents unnecessary processing and provides consistent behavior.
src/utils/mapsV2/zoomToPolygonPlantLocation.tsx (2)
1-4: LGTM! Clean import organization.
The consolidation and separation of type imports improve code organization and readability.
8-11: LGTM! Improved state management approach.
The transition from setViewState to handleViewStateChange with Partial<ViewState> aligns well with the context-based state management pattern, offering more flexibility while maintaining type safety.
src/features/common/Layout/ProjectsLayout/ProjectsLayout.module.scss (1)
52-52: LGTM! Verify layout across different screen sizes.
The 2px increase in top spacing aligns with the PR objectives to improve positioning of map controls. However, since this change affects all screen sizes, not just mobile views, please verify that the layout remains consistent across different viewport dimensions.
Let's verify the impact on different screen sizes:
✅ Verification successful
Based on the search results, I can now make a determination about the review comment. The search revealed:
- The
.mapFeatureExplorerOverlayclass in ProjectsLayout.module.scss is positioned absolutely with the modified top position - The codebase has extensive responsive design patterns using mixins for different screen sizes (xs, sm, md, lg, xl)
- No direct conflicts or dependencies were found for this specific overlay positioning
- The change appears to be isolated to this specific component's styling
LGTM - The 2px spacing adjustment is safe and maintains responsive layout
The increased top spacing from 8px to 10px is a minor adjustment that:
- Maintains the consistent use of the
$layoutPaddingTopvariable - Doesn't interfere with any media queries or responsive breakpoints
- Is applied uniformly across all screen sizes as intended
- Aligns with the PR's goal of improving map control positioning
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any responsive design breakpoints or screen-size specific styles that might be affected by this change
# Test 1: Look for responsive breakpoints in SCSS files
echo "Checking for responsive breakpoints that might affect the overlay positioning..."
rg -A 2 "@include.*\b(xs|sm|md|lg|xl)" --type scss
# Test 2: Search for related mobile-specific styles
echo "Checking for mobile-specific styles related to map controls..."
rg -A 5 "mapFeatureExplorerOverlay|zoom.*button|map.*control" --type scss
Length of output: 492
Script:
#!/bin/bash
# Let's try searching without file type restriction since scss is not recognized
# First, find all scss files
echo "Finding SCSS files..."
fd ".*\.scss$"
# Then search for responsive breakpoints and map-related styles
echo -e "\nChecking for responsive breakpoints..."
rg -A 2 "@include.*\b(xs|sm|md|lg|xl)" $(fd ".*\.scss$")
echo -e "\nChecking for map-related styles..."
rg -A 5 "mapFeatureExplorerOverlay|zoom.*button|map.*control" $(fd ".*\.scss$")
# Also check for any media queries that might affect the layout
echo -e "\nChecking for media queries..."
rg "@media" $(fd ".*\.scss$")
Length of output: 149094
src/utils/mapsV2/zoomToProjectSite.ts (2)
24-24: LGTM! Improved function signature.
The renamed parameter and updated type signature better reflect the function's purpose and provide more flexibility with Partial<ViewState>.
68-68: LGTM! Verify context integration.
The view state update is correctly implemented. Since this is part of a larger transition to context-based state management, let's verify the integration.
✅ Verification successful
Let me gather more specific information about the view state management integration.
View state management integration is properly implemented
The verification confirms that:
- The
handleViewStateChangecallback is correctly integrated with the context-based state management inProjectsMapContext SingleProjectViewproperly consumes the context through theuseProjectsMaphook- The view state updates are consistently handled across zoom operations (project sites, locations, and coordinates)
- The state management follows the context pattern with appropriate typing and default values
The implementation maintains a clean separation of concerns while ensuring proper state synchronization through the context system.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the usage of handleViewStateChange across the codebase
# to ensure consistent context-based state management.
# Search for components using this utility function
echo "Components using zoomInToProjectSite:"
rg -l "zoomInToProjectSite"
# Check the implementation of handleViewStateChange in these components
echo "\nContext usage in components:"
ast-grep --pattern 'const $_ = useContext($_)'
Length of output: 6174
Script:
#!/bin/bash
# Check the implementation details in the relevant files
# Check SingleProjectView implementation
echo "SingleProjectView implementation:"
rg -A 10 "handleViewStateChange" src/features/projectsV2/ProjectsMap/SingleProjectView.tsx
# Check for map context usage
echo "\nMap context usage in SingleProjectView:"
ast-grep --pattern 'const $_ = useContext(ProjectsMapContext)' src/features/projectsV2/ProjectsMap/SingleProjectView.tsx
# Check ProjectsMapContext implementation
echo "\nProjectsMapContext view state handling:"
rg -A 5 "viewState" src/features/projectsV2/ProjectsMapContext.tsx
Length of output: 2687
src/features/projectsV2/ProjectsMap/ProjectsMap.module.scss (2)
85-85: LGTM! Fixes zoom button visibility on Android.
The increased bottom spacing prevents the zoom controls from being obscured by the footer on Android devices.
89-89: LGTM! Optimizes zoom button positioning for iOS.
The reduced bottom spacing provides better balance while maintaining visibility of the zoom controls on iOS devices.
src/features/projectsV2/ProjectsMap/MultipleProjectsView.tsx (3)
16-18: Clean implementation of context consumption!
The component properly consumes both contexts (useProjects and useProjectsMap) with clean destructuring patterns while maintaining proper error handling.
Line range hint 66-69: Clean and focused render logic!
The component maintains a clean and focused render method, passing only the necessary props to ProjectMarkers.
Line range hint 24-37: Consider handling potential race conditions and effect dependencies.
While the deferred execution pattern is good for ensuring map initialization, there are two concerns:
- Race Condition: Multiple effect executions could lead to race conditions in map initialization. Consider adding a cleanup function to cancel pending operations.
- Dependencies: The empty dependency array might miss necessary updates. Consider including
handleViewStateChangeandmapRefas dependencies.
Consider applying this improvement:
- useEffect(() => {
+ useEffect(() => {
+ let isMounted = true;
Promise.resolve().then(() => {
- if (mapRef.current) {
+ if (isMounted && mapRef.current) {
const map = mapRef.current.getMap
? mapRef.current.getMap()
: mapRef.current;
zoomOutMap(map, () => {
handleViewStateChange({
...map.getCenter(),
zoom: map.getZoom(),
});
});
}
});
+ return () => {
+ isMounted = false;
+ };
- }, []);
+ }, [handleViewStateChange, mapRef]);src/features/projectsV2/ProjectsMap/ProjectSiteDropDown/SiteDropdown.module.scss (1)
18-19: LGTM! Improved dropdown positioning.
The transition from hardcoded values to calculated positions using layout variables enhances consistency and maintainability. The additional 10px offset provides appropriate visual spacing.
src/features/projectsV2/ProjectsMapContext.tsx (2)
9-13: LGTM! Well-designed interface extension.
The ExtendedViewState interface appropriately extends ViewState with flexible width/height types, supporting both percentage and pixel values. This change aligns with the PR objective of improving map container responsiveness.
77-84: 🛠️ Refactor suggestion
Review the forced dimension override approach.
The current implementation always forces width/height to '100%', which might prevent legitimate dimension changes needed for responsive layouts or specific container requirements.
Consider a more flexible approach:
const handleViewStateChange = (newViewState: Partial<ExtendedViewState>) => {
setViewState((prev) => ({
...prev,
...newViewState,
- width: '100%', // Always ensure width and height are set
- height: '100%',
+ width: newViewState.width ?? prev.width ?? '100%',
+ height: newViewState.height ?? prev.height ?? '100%',
}));
};src/features/projectsV2/ProjectsMap/SingleProjectView.tsx (1)
24-24: Good rename to better reflect the function's purpose!
The change from setViewState to handleViewStateChange better describes the function's intent and follows React's event handling naming conventions.
src/features/projectsV2/ProjectDetails/components/KeyInfo.tsx (3)
38-47: Well-structured visibility flags improve code maintainability!
The introduction of clear, descriptive boolean flags at the top of the component effectively centralizes the visibility logic and makes the code more maintainable. Each flag properly handles null checks and ensures meaningful values.
Line range hint 124-134: Consistent implementation of seasons rendering!
The code effectively reuses the InterventionSeason component for both activity and planting seasons, maintaining consistency with the visibility flag pattern established above.
48-48: Verify date formatting implementation.
The code now uses the formatDate utility instead of a local implementation. Let's verify that this utility handles the required string cleanup and zero-padding mentioned in the PR objectives.
✅ Verification successful
Let me check the implementation of the formatDate utility to verify it handles the required formatting.
Let me see the complete implementation to verify the zero-padding and formatting logic.
Date formatting implementation is properly handled
The formatDate utility correctly implements the required functionality:
- Cleans up date strings by removing time portion
- Handles zero-padding for month and day using
padStart(2, '0') - Uses
date-fnsfor consistent formatting with localization support - Returns dates in the format "Month DD, YYYY"
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for the formatDate utility implementation
rg -l "function.*formatDate" src/utils/
Length of output: 87
Script:
#!/bin/bash
# Get the implementation of formatDate
rg -A 10 "function.*formatDate" src/utils/countryCurrency/getFormattedDate.ts
Length of output: 435
Script:
#!/bin/bash
# Get the complete implementation of formatDate
cat src/utils/countryCurrency/getFormattedDate.ts
Length of output: 1028
src/features/projectsV2/ProjectsMap/index.tsx (3)
1-1: LGTM: Import changes enhance type safety and performance
The addition of ViewStateChangeEvent type and useDebouncedEffect hook are good improvements for type safety and performance optimization.
Also applies to: 25-25
42-43: LGTM: Improved view state management
The transition from direct state setting to a handled approach with handleViewStateChange is a good improvement. The onMove callback is properly memoized and typed.
Also applies to: 105-110
Line range hint 181-189: Consider dynamic navigation control positioning for mobile devices
The fixed bottom-right position for navigation controls might still cause issues on mobile devices. Consider adjusting the position dynamically based on the device type to prevent the controls from being obscured by the footer.
-<NavigationControl position="bottom-right" showCompass={false} />
+<NavigationControl
+ position={props.isMobile ? "bottom-left" : "bottom-right"}
+ showCompass={false}
+/>- also debounces the useEffect that runs on updates to `filteredProjects` to prevent excessive map centering - adds a check to prevent map centering if position won't significantly change
mohitb35
force-pushed
the
projects-redesign/launch-feedback
branch
from
5f060ac to
a07e8d5
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
src/features/projectsV2/ProjectsMap/index.tsx (2)
9-9: Consider addressing the Map component shadowing.The Map import shadows the global Map object. While this is common practice with mapping libraries, it's better to be explicit.
Consider renaming to:
-import Map from 'react-map-gl-v7/maplibre'; +import MapGL from 'react-map-gl-v7/maplibre';🧰 Tools
🪛 Biome
[error] 9-9: Do not shadow the global "Map" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
58-83: Consider extracting complex conditions for better readability.The map centering logic is well-implemented with proper checks and debouncing. However, the complex conditions could be more readable.
Consider this refactor:
useDebouncedEffect( () => { const map = mapRef.current; + const isValidProjectCount = + filteredProjects !== undefined && + filteredProjects.length > 0 && + (filteredProjects.length < 30 || filteredProjects.length === projects?.length); + - const shouldCenterMap = - filteredProjects !== undefined && - filteredProjects.length > 0 && - (filteredProjects.length < 30 || - filteredProjects.length === projects?.length) && - map !== null; + const shouldCenterMap = isValidProjectCount && map !== null; if (!shouldCenterMap) return; const validFeatures = getValidFeatures(filteredProjects); if (validFeatures.length === 0) return; const centroid = calculateCentroid(validFeatures); if (!centroid.geometry) return; const currentCenter = map.getCenter(); if (areMapCoordsEqual(currentCenter, centroid.geometry.coordinates)) return; centerMapOnCoordinates(mapRef, centroid.geometry.coordinates); }, 250, [filteredProjects] );src/utils/projectV2.ts (1)
224-242: LGTM! Consider extracting the epsilon constant.The implementation is clean, well-documented, and serves its purpose in preventing unnecessary map re-centering. However, consider extracting the epsilon constant to improve maintainability.
+// Distance threshold in degrees (approximately 10 meters at the Equator) +const MAP_COORDS_EPSILON = 0.00009; + export const areMapCoordsEqual = ( mapCenter: { lng: number; lat: number } | undefined, centroidCoords: Position, - epsilon = 0.00009 // Very small difference threshold corresponding to 10m at Equator + epsilon = MAP_COORDS_EPSILON ): boolean => {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
package.jsonis excluded by!**/*.json
📒 Files selected for processing (3)
.gitignore(1 hunks)src/features/projectsV2/ProjectsMap/index.tsx(6 hunks)src/utils/projectV2.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .gitignore
🧰 Additional context used
🪛 Biome
src/features/projectsV2/ProjectsMap/index.tsx
[error] 9-9: Do not shadow the global "Map" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
🔇 Additional comments (4)
src/features/projectsV2/ProjectsMap/index.tsx (3)
43-44: LGTM! Improved state management approach.
The transition from direct state setting to a handled approach provides better control over view state updates.
112-117: LGTM! Well-structured event handler.
The onMove handler is properly memoized and uses the new handleViewStateChange approach consistently.
188-188: LGTM! Consistent event handling.
The Map component's onMove prop correctly uses the new callback.
src/utils/projectV2.ts (1)
224-242: Verify the function's usage in map-related components.
Let's verify that this function is being used correctly in the components that handle map centering and filtering.
✅ Verification successful
Function is correctly used for map centering logic
The verification shows that areMapCoordsEqual is properly implemented and used in the ProjectsMap component to prevent unnecessary re-centering of the map. It compares the current map center with centroid coordinates before triggering the centering operation, which is an appropriate optimization.
- Used in
src/features/projectsV2/ProjectsMap/index.tsxas part of a debounced map centering logic - No other coordinate comparison functions exist in the utils that could cause conflicts
- Implementation follows best practices for coordinate comparison with epsilon threshold
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find components that use areMapCoordsEqual for map centering logic
# Test 1: Find direct usage of areMapCoordsEqual
echo "=== Direct usage of areMapCoordsEqual ==="
rg "areMapCoordsEqual" --type ts --type tsx -l
# Test 2: Find components that might need this function
echo -e "\n=== Components handling map center or filtering ==="
rg -A 5 "setViewState|handleViewStateChange" --type tsx
Length of output: 372
Script:
#!/bin/bash
# Let's try without file type restrictions since tsx might not be recognized
# Find direct usage of areMapCoordsEqual
echo "=== Direct usage of areMapCoordsEqual ==="
rg "areMapCoordsEqual" -l
# Find components handling map view state
echo -e "\n=== Components handling map view state ==="
rg -l "(setViewState|handleViewStateChange|mapCenter)"
# Look for map-related components
echo -e "\n=== Map-related components ==="
fd -e tsx -e ts "Map"
Length of output: 2411
Script:
#!/bin/bash
# Let's examine how areMapCoordsEqual is used in ProjectsMap component
echo "=== Usage in ProjectsMap ==="
rg -A 10 "areMapCoordsEqual" src/features/projectsV2/ProjectsMap/index.tsx
# Let's also check the context of map center handling
echo -e "\n=== Map center handling context ==="
rg -B 5 -A 10 "mapCenter" src/features/projectsV2/ProjectsMap/index.tsx
# Check if there are any other coordinate comparison functions
echo -e "\n=== Other coordinate comparison functions ==="
rg "(isEqual|compare|equals.*coord)" src/utils/
Length of output: 1663
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (5)
src/features/projectsV2/ProjectsMap/ProjectSiteDropDown/ProjectSiteList.tsx (1)
52-54: Consider enhancing the area formatting for better internationalization and precision.While the locale-aware formatting is a good improvement, consider these enhancements:
- Internationalize the "ha" unit using the translation system
- Consider showing decimals for small areas (< 1 ha)
- Extract the formatting logic to a separate function for better maintainability
- <p className={styles.siteArea}> - {getFormattedRoundedNumber(locale, site.siteArea, 0)} ha - </p> + <p className={styles.siteArea}> + {formatSiteArea(locale, site.siteArea)} + </p> + // Add this function above the component + const formatSiteArea = (locale: string, area: number) => { + const decimals = area < 1 ? 2 : 0; + const formattedArea = getFormattedRoundedNumber(locale, area, decimals); + // Assuming you have a translation function like t() + const unit = t('common.units.hectare'); + return `${formattedArea} ${unit}`; + };src/features/projectsV2/ProjectDetails/styles/PlantLocationInfo.module.scss (1)
180-183: LGTM! Consider consolidating scientific name styles.The addition of italic styling for scientific names follows proper typographic conventions.
Consider consolidating all scientific name styles into a single shared class to maintain consistency, as there are multiple instances of scientific name styling in this file (e.g.,
.scientificNameon line 139). Here's a suggested approach:- .scientificName .data { - font-style: italic; - } + @mixin scientific-name { + font-style: italic; + } + + .scientificName .data { + @include scientific-name; + }Then update other scientific name styles to use this mixin for consistency.
src/features/projectsV2/ProjectDetails/components/microComponents/PlantInfoCard.tsx (1)
Line range hint
41-43: Fix incorrect logical condition in shouldRenderThe current condition
tag !== undefined || tag !== nullwill always evaluate to true because any value will either be different from undefined OR different from null. This means the tag will always render, even when it shouldn't.Apply this fix:
{ label: tProjectDetails('treeTag'), data: tProjectDetails('tag', { number: tag, }), - shouldRender: tag !== undefined || tag !== null, + shouldRender: tag !== undefined && tag !== null, },src/features/projectsV2/ProjectsMap/ProjectSiteDropDown/index.tsx (2)
98-105: Add error handling and consider configuration for decimal placesWhile the formatting implementation is good, consider these improvements:
- Add null/undefined checking for siteArea
- Consider making decimal places configurable based on the area size
Consider this safer implementation:
- <span> - {getFormattedRoundedNumber( - locale, - selectedSiteData?.siteArea, - 0 - )}{' '} - ha - </span> + <span> + {selectedSiteData?.siteArea + ? `${getFormattedRoundedNumber( + locale, + selectedSiteData.siteArea, + selectedSiteData.siteArea < 1 ? 2 : 0 + )} ha` + : '-'} + </span>
Line range hint
42-135: Consider component decomposition for better maintainabilityWhile the current implementation works well, consider these architectural improvements for future iterations:
- Extract the site area display into a separate component
- Consider using a custom hook for site selection logic
- Move dropdown state management to a context if it needs to be shared
This would improve maintainability and make the component more focused on its primary responsibility.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
src/features/projectsV2/ProjectDetails/components/microComponents/PlantInfoCard.tsx(1 hunks)src/features/projectsV2/ProjectDetails/styles/PlantLocationInfo.module.scss(1 hunks)src/features/projectsV2/ProjectsMap/ProjectSiteDropDown/ProjectSiteList.tsx(3 hunks)src/features/projectsV2/ProjectsMap/ProjectSiteDropDown/index.tsx(4 hunks)
🔇 Additional comments (6)
src/features/projectsV2/ProjectsMap/ProjectSiteDropDown/ProjectSiteList.tsx (2)
1-2: LGTM! Imports are correctly added for locale support.
The new imports support the enhancement of number formatting with locale awareness.
32-32: LGTM! Locale initialization is properly implemented.
The useLocale hook is correctly used at the component level for locale-aware formatting.
src/features/projectsV2/ProjectDetails/components/microComponents/PlantInfoCard.tsx (2)
60-62: LGTM: Styling enhancement for scientific names
The addition of the styles.scientificName class improves the visual distinction of scientific names.
Line range hint 1-96: Consider enhancing error handling for date formatting
The component uses formatDate utility without error handling. While the date is checked for existence, invalid date strings could cause runtime errors.
Let's check the implementation of formatDate:
Consider wrapping the date formatting in a try-catch block:
{
label: tProjectDetails('plantingDate'),
- data: interventionStartDate ? formatDate(interventionStartDate) : null,
+ data: interventionStartDate ?
+ (() => {
+ try {
+ return formatDate(interventionStartDate);
+ } catch (error) {
+ console.error('Error formatting date:', error);
+ return null;
+ }
+ })()
+ : null,
shouldRender: interventionStartDate !== undefined,
},src/features/projectsV2/ProjectsMap/ProjectSiteDropDown/index.tsx (2)
9-9: LGTM: Import additions for localization support
The new imports support the enhancement of number formatting with locale-specific handling.
Also applies to: 18-18
56-56: LGTM: Proper locale hook initialization
The useLocale hook is correctly initialized at the component level for consistent locale-based formatting.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
src/features/projectsV2/ProjectDetails/components/microComponents/SpeciesPlanted.tsx (1)
38-38: Consider adding defensive checks for edge cases.While the number formatting implementation is good, consider handling edge cases for better robustness.
- <p>{getFormattedNumber(locale, species.treeCount)}</p> + <p>{species.treeCount != null ? getFormattedNumber(locale, species.treeCount) : '-'}</p>This change ensures graceful handling of potential null or undefined values.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
src/features/projectsV2/ProjectDetails/components/microComponents/SpeciesPlanted.tsx(3 hunks)src/features/projectsV2/ProjectsMap/ProjectsMap.module.scss(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/features/projectsV2/ProjectsMap/ProjectsMap.module.scss
🔇 Additional comments (3)
src/features/projectsV2/ProjectDetails/components/microComponents/SpeciesPlanted.tsx (3)
5-6: LGTM! Import statements are properly structured.
The new imports for locale handling and number formatting align well with the PR's objective to enhance data presentation.
15-15: LGTM! Proper hook usage for locale initialization.
The locale initialization is correctly placed and follows React hooks best practices.
Line range hint 1-47: Verify consistent number formatting across the application.
The number formatting changes look good, but we should ensure consistency across other components that display numbers.
✅ Verification successful
Number formatting is consistently implemented across the application
Based on the search results, the getFormattedNumber utility is already widely used across the application for number formatting, including:
- Tree counts in leaderboards
- Project statistics and metrics
- Analytics components
- Plant location details
- Species counts and percentages
The implementation in SpeciesPlanted.tsx follows the established pattern of using getFormattedNumber for displaying numeric values, consistent with other components in the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for direct number rendering in other components
# Look for potential places where getFormattedNumber should be used
# Search for components that might need similar formatting
rg -t tsx -t jsx "{\s*[a-zA-Z]*Count\s*}" --glob "src/features/**"
# Search for other numeric displays that might need formatting
rg -t tsx -t jsx "toFixed|toString|Number\." --glob "src/features/**"
Length of output: 266
Script:
#!/bin/bash
# Search for components that might need similar formatting
rg "\\{\\s*\\w+Count\\s*\\}" --type=ts --type=tsx src/features/
# Search for other numeric displays that might need formatting
rg "toFixed\\(|toString\\(|Number\\." --type=ts --type=tsx src/features/
# Search for existing usage of getFormattedNumber
rg "getFormattedNumber" --type=ts --type=tsx
Length of output: 261
Script:
#!/bin/bash
# Search for components that might need similar formatting
rg "\\{\\s*\\w+Count\\s*\\}"
# Search for other numeric displays that might need formatting
rg "toFixed\\(|toString\\(|Number\\."
# Search for existing usage of getFormattedNumber
rg "getFormattedNumber"
Length of output: 23876
Uh oh!
There was an error while loading. Please reload this page.