New explore dropdown UI #2336
New explore dropdown UI #2336
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
WalkthroughThe pull request introduces extensive mobile-responsive enhancements to the Map Feature Explorer in the Projects V2 section. Key changes include the creation of new components such as Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Pro Plan! Simply use the command ✨ Finishing Touches
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
src/features/projectsV2/ProjectsMap/MapFeatureExplorer/MapSettings.tsx (2)
28-28: Remove unnecessary div nestingThe extra
<div>wrapper appears to serve no semantic or styling purpose and can be removed to improve code clarity.- <div> <div> <MapLayerToggle // ... component content /> </div> <div className={styles.exploreDescription}> {tMaps('3trilliontrees')} </div> - </div>
Line range hint
31-57: Remove commented-out code blocksLarge blocks of commented code make the file harder to maintain and understand. If these features are planned for future implementation, consider:
- Tracking them in issues/tickets
- Relying on version control history
- Creating separate feature branches
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/features/projectsV2/ProjectsMap/MapFeatureExplorer/MapFeatureExplorer.module.scss(2 hunks)src/features/projectsV2/ProjectsMap/MapFeatureExplorer/MapSettings.tsx(1 hunks)
🔇 Additional comments (3)
src/features/projectsV2/ProjectsMap/MapFeatureExplorer/MapFeatureExplorer.module.scss (3)
15-15: Verify the visual impact of reduced padding
The padding has been significantly reduced from 14px 16px to a uniform 8px. This could make the content feel cramped, especially given the fixed max-width: 182px.
Please confirm that:
- Content has sufficient breathing room
- Text remains readable
- Interactive elements have enough touch targets
24-24: LGTM! Enhanced visual hierarchy
The new styles improve the visual hierarchy with:
- Well-defined padding
- Subtle background color using rgba
- Consistent border-radius
Also applies to: 29-30
89-90: Verify the font size variable exists
The font size has been changed to $fontXXXSmallNew. Please ensure this variable is defined in the theme.
✅ Verification successful
Font size variable is properly defined
The $fontXXXSmallNew variable is correctly defined in src/theme/_fonts.scss as var(--font-xxx-extra-small-new) and is already being used in other components within the project.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the font size variable is defined in theme files
rg -l '\$fontXXXSmallNew' 'src/theme/**/*'
Length of output: 99
Script:
#!/bin/bash
# Let's first find any SCSS/CSS files that might define this variable
fd -e scss -e css | xargs rg '\$fontXXXSmallNew'
# Also search for any potential definition or usage of this font size variable
rg '\$fontXXXSmallNew'
Length of output: 911
Used MUI Modal to render explore features for better compatibility on mobile devices. Provided a cross icon to close the modal view for improved usability.
…project layer configurations -Implemented LayerSwitchContainer component for reusable layer switch functionality. -Added MapLayerControlPanel for structured display of layer categories and configurations. -Created ExploreDropdownHeaderMobile for mobile-specific dropdown header functionality. -Added MapSettings component with forest, soil, and project layer configurations. -Integrated reusable layer configurations with translations and custom styles. -Refactored existing layer toggle components for improved modularity and reusability. -Incorporated conditional rendering for mobile and desktop layouts.
- Updated `.exploreMainContainer` to include `max-height` and hide scrollbars using `::-webkit-scrollbar`. - Introduced a new `.exploreItemsContainer` class with `max-height` and `overflow-y: auto` to enable scrolling for the content. - Adjusted styles for responsiveness in `xsPhoneView` to support dynamic heights for mobile views.
sunilsabatp
force-pushed
the
feature/new-explore-ui
branch
from
d88dbb7 to
58456f8
Compare
…onality for potential future use.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (19)
src/features/projectsV2/ProjectsMap/MapFeatureExplorer/microComponents/ExploreDropdownHeaderMobile.tsx (4)
1-7: Ensure resilience against missing translationsIn case the
'explore'key is missing in the translations, consider a fallback string or a safe check to avoid potential runtime errors.
8-10: Props naming consistencyThe interface name
Propsis too generic. To maintain clarity and consistency with other files, consider naming itExploreDropdownHeaderMobileProps.-interface Props { +interface ExploreDropdownHeaderMobileProps { setIsOpen: SetState<boolean>; }
12-25: Appropriate ARIA attributes for accessibilityWhen using interactive elements such as the close button, consider adding relevant ARIA attributes, like
aria-label="Close"or using a textual alternative for screen readers, to improve accessibility.
27-27: Exporting multiple itemsIf you anticipate adding more exports (like constants or helper functions), consider consolidating them under named exports for modularity and easier refactoring in the future.
src/features/projectsV2/ProjectsMap/MapFeatureExplorer/microComponents/MapLayerControlPanel.tsx (2)
8-15: Consider optional chaining improvementsIf the
source,covariates, or other fields may be absent from the data model in the future, consider optional chaining and default values, particularly when rendering or performing operations on these fields.
69-69: Export statement for flexible importsExporting multiple named exports along with the default could help reuse this component’s types/utility functions in other parts of the codebase without extra imports.
src/features/projectsV2/ProjectsMap/MapFeatureExplorer/microComponents/LayerInfoPopupContent.tsx (2)
7-12: Prop naming clarity
anchorElandsetAnchorElmight be more descriptive astooltipAnchorElorpopupAnchorEl, making the UI purpose clearer.
68-68: Consistent export namingAs in other files, consider matching the component name with the default export for uniformity across your codebase.
src/features/projectsV2/ProjectsMap/MapFeatureExplorer/index.tsx (4)
11-11: Check for large bundle sizes with Material-UI importsModal from
@mui/materialis straightforward, but ensure minimal bundling. Potentially usedynamic()or a code-splitting strategy if the modal is not frequently used.
103-103: Defaulting boolean propsConsider providing a default to
isMobilein the destructuring, e.g.isMobile = false, to simplify conditionals below.- isMobile, + isMobile = false,
117-117: Separation of concernsInstead of inline rendering, consider extracting this logic into a small function or separate component for readability and to facilitate future changes (e.g., toggling different map feature bars).
123-132: Use ARIA attributes for modalsWhen rendering the
<Modal>conditionally, ensure you include attributes such asaria-labelledbyoraria-describedbyto enhance accessibility for screen reader users.src/features/projectsV2/ProjectListControls/ProjectListControlForMobile.tsx (1)
136-136: Use a descriptive prop name or documentation for clarity.
By hardcodingisMobile={true}, you're clearly flagging that the component should adapt its behavior for mobile. However, consider adding a comment or clarifying docstring to emphasize why the mobile override is being forced here (versus deriving it from a responsive hook or global state).src/features/projectsV2/ProjectsMap/MapFeatureExplorer/MapFeatureExplorer.module.scss (3)
12-21: Ensure consistent overflow handling for extra content.
By settingmax-height: 485px;and hiding scrollbars (display: none;), users with long content may not realize there's more to scroll. Consider adding a subtle visual scroll indicator or an alternative approach that ensures discoverability.
33-44: Improve naming clarity.
Renaming.exploreItemsContainerto something more descriptive (e.g.,.exploreContentScrollContainer) might help future maintainers quickly grasp its purpose.
46-66: Add consistent spacing constants.
The.exploreFeatureMobileHeaderhas hardcoded padding, gap, and font sizes. Consider referencing shared spacing tokens (e.g.,$spacingSmall,$fontSmall) for consistent design.src/features/projectsV2/ProjectsMap/MapFeatureExplorer/MapSettings.tsx (3)
1-3: Clean up leftover import references.
You have new imports (SetState) and old ones (ChangeEventorStyledSwitch) commented out. Remove no-longer-used imports to keep the codebase lean.
88-96: Remove commented-out toggles if not needed.
Code references toMapLayerToggleorStyledSwitchmight become outdated quickly. Trim them if you don't plan to reintroduce them soon.
128-131: Extend modular approach to future config expansions.
UsingexploreConfigarrays is a great pattern. Document how devs can add more objects (e.g., new toggles, new layers) without duplicating logic in multiple places.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
public/static/locales/en/maps.jsonis excluded by!**/*.json
📒 Files selected for processing (8)
src/features/projectsV2/ProjectListControls/ProjectListControlForMobile.tsx(1 hunks)src/features/projectsV2/ProjectsMap/MapFeatureExplorer/MapFeatureExplorer.module.scss(2 hunks)src/features/projectsV2/ProjectsMap/MapFeatureExplorer/MapSettings.tsx(3 hunks)src/features/projectsV2/ProjectsMap/MapFeatureExplorer/index.tsx(3 hunks)src/features/projectsV2/ProjectsMap/MapFeatureExplorer/microComponents/ExploreDropdownHeaderMobile.tsx(1 hunks)src/features/projectsV2/ProjectsMap/MapFeatureExplorer/microComponents/LayerInfoPopupContent.tsx(1 hunks)src/features/projectsV2/ProjectsMap/MapFeatureExplorer/microComponents/LayerSwitchContainer.tsx(1 hunks)src/features/projectsV2/ProjectsMap/MapFeatureExplorer/microComponents/MapLayerControlPanel.tsx(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/features/projectsV2/ProjectsMap/MapFeatureExplorer/microComponents/LayerSwitchContainer.tsx
🔇 Additional comments (11)
src/features/projectsV2/ProjectsMap/MapFeatureExplorer/microComponents/MapLayerControlPanel.tsx (1)
17-28: Ensure type safety in the mapOptions callbackThe
updateMapOptionfunction sets abooleanvalue for options. If yourMapOptionstype can accept other types for different fields, consider stricter type checks or a more generalized approach.src/features/projectsV2/ProjectsMap/MapFeatureExplorer/index.tsx (1)
97-97: Optional prop usage consistency
isMobileis optional. Ensure that any internal logic or child components referencingisMobilealso handleundefinedcorrectly, to avoid runtime issues.src/features/projectsV2/ProjectsMap/MapFeatureExplorer/MapFeatureExplorer.module.scss (6)
25-29: Double-check phone resolution constraints.
Positioning the container absolutely in smaller viewports can lead to overlapping or unexpected layout artifacts on certain devices. Verify with a broad range of breakpoints to ensure a stable layout.
67-79: Double-check background color usage.
The subtle green background (rgba(0, 122, 73, 0.05)) might conflict with certain themes. Ensure this color aligns with brand palettes or configurable theme tokens.
80-95: Ensure accessible separation for dashed borders.
While a dashed HR provides visual separation, consider users with low vision. Confirm the contrast ratio meets accessibility standards or provide an alternative.
96-110: Keep consistent sizing with modals.
.layerInfoPopupContaineratmax-width: 189px;may appear too narrow on certain device sizes. Check the final design to confirm readability of text within the popup.
116-116: Standardize container styling.
The newly added background and padding on.toggleMainContainerlook good. Just ensure you apply the same styling approach used in.exploreItemSectionor other containers for consistent layout patterns across the module.Also applies to: 121-121
181-181: Confirm consistent use of extra-small font.
font-size: var(--font-xx-extra-small);is beneficial for space-limited UIs, but it can hinder readability on mobile devices. Verify that it meets accessibility guidelines.src/features/projectsV2/ProjectsMap/MapFeatureExplorer/MapSettings.tsx (3)
12-13: Good practice: Keep module imports collocated.
It's nice to see important modules likeMapLayerControlPanelandExploreDropdownHeaderMobilegrouped together. Maintaining consistent import ordering helps clarity.
18-19: Validate usage of optional props.
isMobile?andsetIsOpen?are helpful for flexible design. Ensure internal logic gracefully handles the absence of these props if not explicitly passed in.
22-27: Nice decoupling for responsive behavior.
This approach of passingisMobileandsetIsOpenkeeps concerns isolated in the parent while allowing the child component to adapt. Good job.
src/features/projectsV2/ProjectsMap/MapFeatureExplorer/microComponents/MapLayerControlPanel.tsx
Outdated
Show resolved
Hide resolved
...features/projectsV2/ProjectsMap/MapFeatureExplorer/microComponents/LayerInfoPopupContent.tsx
Outdated
Show resolved
Hide resolved
src/features/projectsV2/ProjectsMap/MapFeatureExplorer/MapSettings.tsx
Outdated
Show resolved
Hide resolved
|
| .exploreFeatureMobileHeader { | ||
| display: flex; | ||
| justify-content: space-between; | ||
| padding: 3px 4px 18px; |
There was a problem hiding this comment.
There are minor differences from the designed spacing, any reason for that?
As per the design, total spacing of the contents of this component from the top/side is 16px, and bottom is 20px. To achieve that, we would use padding: 4px 4px 20px; (as the uniform padding outside exploreMainContainer is 12px).
mohitb35
force-pushed
the
feature/new-explore-ui
branch
from
e294d54 to
192ecf4
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/features/projectsV2/ProjectsMap/MapFeatureExplorer/microComponents/LayerInfoPopupContent.tsx (2)
28-28: Extract timeout value to a constant.Consider extracting the magic number
500to a named constant for better maintainability.+const MOUSE_LEAVE_DELAY_MS = 500; + const LayerInfoPopupContent = ({ // ... }) => { // ... - onMouseLeave={() => setTimeout(handleMouseLeave, 500)} + onMouseLeave={() => setTimeout(handleMouseLeave, MOUSE_LEAVE_DELAY_MS)}
30-72: Consider refactoring repetitive conditional rendering.The component has a repetitive pattern for rendering different sections. Consider refactoring using a map over a configuration object.
+const INFO_SECTIONS = [ + { key: 'dataYears', labelKey: 'additionalInfo.dataYears' }, + { key: 'resolution', labelKey: 'additionalInfo.resolution' }, + { key: 'description', labelKey: 'additionalInfo.description' }, + { key: 'underlyingData', labelKey: 'additionalInfo.underlyingData' }, +] as const; + const LayerInfoPopupContent = ({ // ... }) => { // ... return ( <div className={styles.layerInfoPopupContainer} /* ... */> - {additionalInfo?.dataYears && ( - <div> - <p className={styles.label}>{tExplore('additionalInfo.dataYears')}</p> - <p>{additionalInfo?.dataYears}</p> - </div> - )} - // ... repeated for resolution, description, underlyingData + {INFO_SECTIONS.map(({ key, labelKey }) => + additionalInfo?.[key] && ( + <div key={key}> + <p className={styles.label}>{tExplore(labelKey)}</p> + <p>{additionalInfo[key]}</p> + </div> + ) + )} {additionalInfo?.source && ( // ... source section remains unchanged )} </div> ); };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/features/projectsV2/ProjectsMap/MapFeatureExplorer/MapFeatureExplorer.module.scss(1 hunks)src/features/projectsV2/ProjectsMap/MapFeatureExplorer/MapSettings.tsx(1 hunks)src/features/projectsV2/ProjectsMap/MapFeatureExplorer/microComponents/LayerInfoPopupContent.tsx(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
src/features/projectsV2/ProjectsMap/MapFeatureExplorer/microComponents/LayerInfoPopupContent.tsx (1)
Learnt from: mohitb35
PR: Plant-for-the-Planet-org/planet-webapp#2336
File: src/features/projectsV2/ProjectsMap/MapFeatureExplorer/microComponents/LayerInfoPopupContent.tsx:0-0
Timestamp: 2025-01-31T10:29:59.907Z
Learning: In the Plant-for-the-Planet webapp, URLs in mapSettings.config.ts are hardcoded and don't require validation as they are trusted developer-defined values, not user input.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (javascript)
🔇 Additional comments (5)
src/features/projectsV2/ProjectsMap/MapFeatureExplorer/MapSettings.tsx (3)
17-22: Address unused props flagged by static analysis.The
mapOptionsandupdateMapOptionprops are marked as unused in the component's signature.
23-55: LGTM! Well-structured modular implementation.The code has been nicely refactored to use
MapSettingsSectioncomponents with configuration frommapSettingsConfig. This modular approach improves maintainability and reusability.
57-63: LGTM! Clean mobile support implementation.The conditional rendering for mobile view is well-implemented, using the
MobileMapSettingsLayoutcomponent when appropriate.src/features/projectsV2/ProjectsMap/MapFeatureExplorer/MapFeatureExplorer.module.scss (2)
112-129: Address accessibility concerns in the info popup.The current implementation has potential accessibility issues:
- Font size (10px) is below WCAG recommendations
- Fixed width might truncate content in some languages
42-46: LGTM! Robust mobile viewport height handling.The implementation uses multiple fallbacks (
100vh,-webkit-fill-available,stretch) to handle mobile viewport height correctly across different browsers.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/features/projectsV2/ProjectsMap/MapFeatureExplorer/MapFeatureExplorer.module.scss (1)
112-129:⚠️ Potential issueAddress accessibility concerns in the info popup.
The current implementation has potential accessibility issues:
- Font size (10px) is below WCAG recommendations
- Fixed width might truncate content in some languages
.layerInfoPopupContainer { display: flex; flex-direction: column; gap: 8px; - max-width: 189px; + max-width: min(189px, 90vw); background: #fff; padding: 12px; - font-size: 10px; + font-size: 12px; .label { font-weight: 600; color: rgba(130, 130, 130, 1); text-transform: uppercase; } .source { color: $primaryDarkColor; + word-break: break-word; } }
🧹 Nitpick comments (3)
src/features/projectsV2/ProjectsMap/MapFeatureExplorer/MapFeatureExplorer.module.scss (3)
10-25: Consider accessibility improvements for button content.While the layout is well-structured, consider adding these accessibility enhancements:
- Ensure sufficient color contrast for text
- Add minimum touch target size for mobile
.exploreButtonContent { display: flex; flex-direction: column; align-items: flex-start; + min-height: 44px; // Minimum touch target size for mobile h3 { font-size: $fontSmall; + color: $primaryDarkColor; // Ensure sufficient contrast @include xsPhoneView { font-size: $fontXXSmall; } } p { font-weight: 400; + color: $primaryDarkColor; // Ensure sufficient contrast } }
69-85: Consider using CSS variables for better maintainability.Extract commonly used values into CSS variables for better maintainability and consistency.
.exploreItemSection { - background: rgba(0, 122, 73, 0.05); + background: var(--color-primary-light); border-radius: 8px; padding: 8px 9px; - font-size: 12px; + font-size: var(--font-xx-small); margin-bottom: 10px; h2 { font-weight: 600; margin-bottom: 6px; font-size: $fontSmall; } }
87-110: Add hover states for better interactivity.Consider adding hover states to improve user interaction feedback.
.singleLayerOption { display: flex; justify-content: space-between; gap: 10px; align-items: center; padding: 6px 0; border-bottom: 1px solid $horizontalLineColor; + transition: background-color 0.2s ease; + + &:hover { + background-color: var(--color-primary-light); + } .layerLabel { cursor: default; &.additionalInfo { cursor: help; p { text-decoration: $horizontalLineColor dashed underline; text-underline-offset: 2px; + &:hover { + text-decoration-color: var(--color-primary); + } } } } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/features/projectsV2/ProjectsMap/MapFeatureExplorer/CustomSwitch.tsx(2 hunks)src/features/projectsV2/ProjectsMap/MapFeatureExplorer/MapFeatureExplorer.module.scss(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (javascript)
🔇 Additional comments (6)
src/features/projectsV2/ProjectsMap/MapFeatureExplorer/CustomSwitch.tsx (3)
2-2: LGTM! Good use of theme system.The addition of themeProperties import promotes consistent theming across the application.
10-11: LGTM! Improved touch targets for better mobile UX.The increased dimensions enhance usability, particularly on mobile devices:
- Larger track and thumb for better visibility
- Increased touch target size for improved interaction
- Proportionally scaled padding and translation
Also applies to: 14-15, 21-23, 26-26
38-38: LGTM! Robust color handling with theme fallback.Good improvement to use theme-based fallback colors when customColor is not provided. This ensures consistent styling and prevents undefined colors.
Also applies to: 41-41
src/features/projectsV2/ProjectsMap/MapFeatureExplorer/MapFeatureExplorer.module.scss (3)
27-52: LGTM! Mobile viewport height implementation looks good.The implementation correctly addresses mobile viewport height issues by using multiple fallbacks:
height: 100vhheight: -webkit-fill-availableheight: stretch
54-67: LGTM! Scrolling behavior implementation aligns with team's decision.The implementation correctly handles overflow with hidden scrollbars as per team's preference.
131-140: LGTM! Good use of CSS variables for font size.The implementation correctly uses CSS variables for font size and has appropriate dimensions for the switch container.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/features/projectsV2/ProjectsMap/MapFeatureExplorer/microComponents/SingleLayerOption.tsx (2)
18-28: Consider optimizing hasInfoPopover calculation.Move the
hasInfoPopoverconstant inside theuseCallbackhooks where it's used to avoid recalculation on every render.- const hasInfoPopover = layerConfig.additionalInfo !== undefined; const [anchor, setAnchor] = useState<HTMLDivElement | null>(null); if (!layerConfig.isAvailable) return null; const handleMouseEnter = useCallback( (e: React.MouseEvent<HTMLParagraphElement>) => { + const hasInfoPopover = layerConfig.additionalInfo !== undefined; setAnchor(e.currentTarget); }, - [] + [layerConfig.additionalInfo] );
29-48: Add error handling and optimize handleClose.Two suggestions for improvement:
- Add error handling for the relatedTarget type assertion
- Memoize the handleClose function
+ const handleClose = useCallback(() => { + setAnchor(null); + }, []); const handleMouseLeave = useCallback((e: MouseEvent) => { - const relatedTarget = e.relatedTarget as HTMLElement; + if (!(e.relatedTarget instanceof HTMLElement)) { + handleClose(); + return; + } + const relatedTarget = e.relatedTarget; const isMovingToPopover = relatedTarget?.closest('[role="presentation"]'); if (!isMovingToPopover) { handleClose(); } - }, []); + }, [handleClose]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/features/projectsV2/ProjectsMap/MapFeatureExplorer/microComponents/SingleLayerOption.tsx(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (javascript)
🔇 Additional comments (2)
src/features/projectsV2/ProjectsMap/MapFeatureExplorer/microComponents/SingleLayerOption.tsx (2)
1-16: LGTM! Well-structured imports and type definitions.The imports are logically organized, and the Props interface is well-defined with proper TypeScript types.
49-107: LGTM! Clean and well-structured render logic.The render logic is well-implemented with:
- Proper conditional rendering
- Well-configured Material-UI Popover
- Proper event handling for the StyledSwitch component
- also adds `key` to type `SingleExploreLayerConfig` - updates `useFetchLayers` to use layer `key` from API instead of converting name to camel case - updates styles to show vertical scrollbar
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/features/projectsV2/ProjectsMapContext.tsx (2)
71-71: Consider using a more specific type for metadata.The
Record<never, never>type for metadata is equivalent to an empty object{}. If metadata is expected to be empty, consider documenting why. If it's expected to have properties in the future, consider using a more specific type or adding a TODO comment.- metadata: Record<never, never>; + metadata: Record<string, unknown>; // TODO: Define specific metadata properties
124-129: Consider optimizing the isExploreMode calculation.The current implementation uses
Object.entrieson everymapOptionschange. Consider optimizing for performance:useEffect(() => { - const enabledLayers = Object.entries(mapOptions).filter( - ([key, value]) => key !== 'projects' && value === true - ); - setIsExploreMode(enabledLayers.length > 0); + const hasEnabledExploreLayers = Object.keys(mapOptions).some( + (key) => key !== 'projects' && mapOptions[key] === true + ); + setIsExploreMode(hasEnabledExploreLayers); }, [mapOptions]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/features/projectsV2/ProjectsMap/MapFeatureExplorer/MapFeatureExplorer.module.scss(1 hunks)src/features/projectsV2/ProjectsMapContext.tsx(5 hunks)src/utils/mapsV2/mapSettings.config.ts(1 hunks)src/utils/mapsV2/useFetchLayers.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/utils/mapsV2/mapSettings.config.ts
- src/features/projectsV2/ProjectsMap/MapFeatureExplorer/MapFeatureExplorer.module.scss
- src/utils/mapsV2/useFetchLayers.ts
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (javascript)
🔇 Additional comments (3)
src/features/projectsV2/ProjectsMapContext.tsx (3)
50-86: Well-structured type definitions!The new type definitions are well-organized and provide good type safety:
MapOptionsuses a mapped type for flexibilitySingleExploreLayerConfigprovides comprehensive layer configurationVisParamsandLayerZoomConfighave good separation of concerns
88-107: Clean interface extension for explore functionality!The additions to
ProjectsMapStateinterface are well-structured and maintain type safety while providing all necessary state management capabilities for the explore dropdown UI.
114-121: Clean state management implementation!The state initialization is well-structured with proper typing and good use of useState hooks.
- mammalsDensity - birdsDensity - amphibiansDensity - fireRisk
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/features/projectsV2/ProjectsMap/MapFeatureExplorer/MapFeatureExplorer.module.scss (2)
29-39: Refine container design in.exploreMainContainer.
The updated properties—max-width: 240px;, a fixed white background (background-color: #fff;), and reduced padding (padding: 12px;)—streamline the visual appearance of the container. Additionally, hiding the scrollbar via the WebKit selector improves aesthetics, assuming that content overflow is managed elsewhere. Consider using a theme or design variable for the background color for better consistency.
47-57: Optimize mobile viewport adjustments in.exploreMainContainer.
The mobile-specific styles (within@include xsPhoneView) employ multiple height declarations (100vh,-webkit-fill-available, andstretch) to mitigate issues with mobile browser toolbars. The configuration also includes amin-width: 293px, tailored border-radius, and overrides likemargin-top: 0;. Please test these settings across target devices to ensure they deliver a reliable and consistent mobile experience.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/features/common/Layout/ProjectsLayout/ProjectsLayout.module.scss(1 hunks)src/features/projectsV2/ProjectsMap/MapFeatureExplorer/CustomButton.tsx(1 hunks)src/features/projectsV2/ProjectsMap/MapFeatureExplorer/MapFeatureExplorer.module.scss(1 hunks)src/features/projectsV2/ProjectsMap/MapFeatureExplorer/MapSettings.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/features/projectsV2/ProjectsMap/MapFeatureExplorer/CustomButton.tsx
- src/features/projectsV2/ProjectsMap/MapFeatureExplorer/MapSettings.tsx
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Analyze (javascript)
- GitHub Check: ESLint on Ubuntu
🔇 Additional comments (8)
src/features/common/Layout/ProjectsLayout/ProjectsLayout.module.scss (1)
56-58: Review the overlay positioning properties.
The addition ofbottom: calc($layoutPaddingBottom + 10px); z-index: 50;ensures that the
.mapFeatureExplorerOverlayhas extra spacing at the bottom and appears above other elements. Verify that these computed values align with the overall design guidelines and perform correctly across different viewport sizes.src/features/projectsV2/ProjectsMap/MapFeatureExplorer/MapFeatureExplorer.module.scss (7)
7-9: Responsive adjustments in.mapFeatureExplorer.
The inclusion ofheight: 100%;andgap: 12px;within the@include smTabletViewblock ensures that the component stretches to fill its container and maintains consistent spacing on tablet screens. Please verify that these settings integrate well with nested components and overall layout constraints.
12-27: Solidify styling for.exploreButtonContent.
The new flex-based layout with nested styling forh3(toggling between$fontSmalland$fontXXSmallfor xsPhoneView) and the defined font weight forpenhances typography scaling on mobile. Ensure that the font size tokens are consistent with your design system across the application.
61-87: Improve scroll styling for.exploreItemsContainer.
Custom scrollbar properties—such asscrollbar-width: thinand tailored WebKit scrollbar settings—provide a more polished scrolling experience. The adjustments (including settingmax-height: nonefor xsPhoneView) are effective as long as they do not lead to unexpected layout expansion on content-heavy screens. Verify behavior across different browsers.
89-105: Ensure consistency in.exploreItemSectionstyling.
The defined background color, border-radius, padding, and font size clearly delineate individual items. The innerh2styling further reinforces consistent typography. Confirm that these settings adhere to your overall spacing and sizing guidelines.
107-130: Review layout in.singleLayerOption.
Flex properties with defined spacing (gap: 10px;) and padding establish a clean separation between elements. The dashed underline for elements with the.additionalInfomodifier adds subtle visual cues. It’s advisable to double-check that the text decoration and cursor styles remain accessible and intuitive.
151-154: Validate updated dimensions in.switchContainer.
Updating the dimensions to a height of 21px and a width of 36px improves the tap target size for interactive elements. Ensure these changes meet mobile accessibility guidelines and function well across different devices.
156-160: Confirm changes in.exploreDescription.
The removal of extra padding and the update to using a CSS variable for the font size promote consistency with theme settings. Verify that the reduced margin and overall spacing still align with the intended design, ensuring legibility and visual balance.
| .layerInfoPopupContainer { | ||
| display: flex; | ||
| margin-top: 8px; | ||
| } | ||
| flex-direction: column; | ||
| gap: 8px; | ||
| max-width: 189px; | ||
| background: #fff; | ||
| padding: 12px; | ||
| font-size: 10px; | ||
|
|
||
| .sliderContainer { | ||
| width: 107px; | ||
| display: flex; | ||
| margin-top: 10px; | ||
| .label { | ||
| font-weight: 600; | ||
| color: rgba(130, 130, 130, 1); | ||
| text-transform: uppercase; | ||
| } | ||
| .source { | ||
| color: $primaryDarkColor; | ||
| } | ||
| } |
There was a problem hiding this comment.
Assess readability in .layerInfoPopupContainer.
The info popup is compactly styled with a small gap and padding; however, the set font size of 10px might be challenging for readability and accessibility. Consider increasing it (for example, to 12px) or confirming that this size is an intentional design choice.
- updates mapOptions handling to store only one active layer - `projects` is an exception and can be seen with other layers
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/features/projectsV2/ProjectsMapContext.tsx (3)
71-71: Consider using a more flexible type for metadata.The
metadatafield usingRecord<never, never>is too restrictive and won't allow any properties. Consider usingRecord<string, unknown>for flexibility or define a specific interface for expected metadata properties.- metadata: Record<never, never>; + metadata: Record<string, unknown>;
124-129: Consider using useMemo for isExploreMode.The
isExploreModecalculation could be optimized usinguseMemoinstead ofuseEffectsince it's a derived state frommapOptions.- useEffect(() => { + const isExploreMode = useMemo(() => { const enabledLayers = Object.entries(mapOptions).filter( ([key, value]) => key !== 'projects' && value === true ); - setIsExploreMode(enabledLayers.length > 0); + return enabledLayers.length > 0; - }, [mapOptions]); + }, [mapOptions]);
155-167: Simplify updateMapOption logic.The function could be simplified while maintaining the same behavior.
const updateMapOption = (option: keyof MapOptions, value: boolean) => { - if (option === 'projects') { - setMapOptions((prevOptions) => ({ - ...prevOptions, - [option]: value, - })); - } else { - setMapOptions((prevOptions) => ({ - projects: Boolean(prevOptions.projects), - [option]: value, - })); - } + setMapOptions((prevOptions) => ({ + ...(option === 'projects' ? prevOptions : { projects: Boolean(prevOptions.projects) }), + [option]: value, + })); };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/features/projectsV2/ProjectsMap/MapFeatureExplorer/microComponents/LayerInfoPopupContent.tsx(1 hunks)src/features/projectsV2/ProjectsMapContext.tsx(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/features/projectsV2/ProjectsMap/MapFeatureExplorer/microComponents/LayerInfoPopupContent.tsx
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Analyze (javascript)
- GitHub Check: ESLint on Ubuntu
🔇 Additional comments (1)
src/features/projectsV2/ProjectsMapContext.tsx (1)
88-107: LGTM! Well-structured interface updates.The additions to
ProjectsMapStateinterface are well-documented and logically organized, providing clear typing for the new explore mode functionality.
- mammalDensity - birdDensity - amphibianDensity
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/utils/mapsV2/mapSettings.config.ts (2)
201-201: Fix unit redundancy in description.The description contains "cg/cm3; cg/kg;" which appears to have a redundant unit. The first unit (cg/cm³) is for bulk density, while the second (cg/kg) appears to be a copy-paste error from another soil property.
- 'cg/cm3; cg/kg; 0-30 cm horizon (A weighted average for all depths)', + 'cg/cm3; 0-30 cm horizon (A weighted average for all depths)',
49-313: Consider adding explicit error handling strategy for API responses.The configuration initializes most layers with
isAvailable: null, indicating they'll be updated based on API responses. Consider documenting how errors in API responses should be handled to maintain a consistent UI experience.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
public/static/locales/en/maps.jsonis excluded by!**/*.json
📒 Files selected for processing (1)
src/utils/mapsV2/mapSettings.config.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Analyze (javascript)
- GitHub Check: ESLint on Ubuntu
🔇 Additional comments (7)
src/utils/mapsV2/mapSettings.config.ts (7)
1-18: Well-structured map layer type definition with clear categorization.The
MapLayerOptionsTypeunion type provides a comprehensive set of options for different environmental data categories, making it easy to reference specific layer types throughout the application.
20-29: Good metadata structure for layer information.The
AdditionalInfointerface provides a well-organized structure for metadata that will be helpful for displaying detailed information about each layer to users.
31-39: Clear interface with good documentation.The
LayerConfiginterface is well-documented with JSDoc comments that explain the purpose of each property, particularly the distinction betweencanShowandisAvailable.
49-51: Clear documentation for configuration object initialization.Good practice to document that
isAvailableis initially set to null and will be updated based on API responses. This helps other developers understand the lifecycle of this configuration object.
52-56: Projects layer is always available by default.Good decision to set
isAvailable: truefor the projects layer, as it appears to be a core feature that should always be accessible to users regardless of API status.
96-96:❓ Verification inconclusive
Verify the resolution value for biomassPotential.
Similar to the forestBiomass layer, this resolution is listed as "~500km" which seems unusually coarse. Consider checking if this should be "~500m" instead.
🏁 Script executed:
#!/bin/bash # Search for any mentions of the correct resolution for biomass potential in the codebase rg -i "biomass potential.*resolution|resolution.*biomass potential" --no-headingLength of output: 82
Action: Confirm BiomassPotential Resolution Value
The resolution for biomassPotential insrc/utils/mapsV2/mapSettings.config.tsis currently set to~500km, which appears unusually coarse compared to what we’d expect based on the forestBiomass layer. Since the automated search didn’t reveal any related configuration or supporting references for biomassPotential, please manually verify whether this value is intentional or if it should be adjusted to~500m.
80-80:❓ Verification inconclusive
Verify the resolution value for forestBiomass.
The resolution is listed as "~500km" which seems unusually coarse compared to other layers that are measured in meters. This might be a typo and should be "~500m" instead.
🏁 Script executed:
#!/bin/bash # Search for any mentions of the correct resolution for forest biomass in the codebase rg -i "forest biomass.*resolution|resolution.*forest biomass" --no-headingLength of output: 76
I'm not seeing evidence that the forest biomass layer uses a different resolution. However, the initial search returned no matches, so I'd like to further verify the context of this configuration file to ensure we correctly interpret the resolution setting.
Let's run a shell script to inspect the context around line 80 and gather all resolution settings in this file:
🏁 Script executed:
#!/bin/bash echo "Displaying lines 70-90 from src/utils/mapsV2/mapSettings.config.ts:" sed -n '70,90p' src/utils/mapsV2/mapSettings.config.ts echo "Searching for all occurrences of resolution settings with context:" rg -n -C 5 "resolution:" src/utils/mapsV2/mapSettings.config.tsLength of output: 7952
Action Required: Confirm the Resolution Unit for Forest Biomass Layers
After reviewing the configuration in
src/utils/mapsV2/mapSettings.config.ts(lines 70–90), it appears that theforestBiomasslayer is currently set with a resolution of~500km. Notably, theforestCoverlayer uses a resolution of~500m (Downscaled), and other layers in the same file are defined in meters or other scales much smaller than kilometers. Additionally, thebiomassPotentiallayer also uses~500km, which suggests a potential inconsistency or typo.
Location:
src/utils/mapsV2/mapSettings.config.ts(lines 75–83 forforestBiomass)- Also, similar resolution appears in the
biomassPotentialconfiguration.Concern:
- The
~500kmvalue for forest biomass(s) seems unusually coarse compared to similar layers (e.g.,forestCoverat~500m).- Given that remote sensing products like MODIS typically operate at the 500-meter level, this discrepancy warrants a careful review.
Please verify whether the resolution value for both
forestBiomassandbiomassPotentialis intended to be set in kilometers or if it should be corrected to~500m.
This PR handles the new explore drop down UI (mobile, desktop).
Also includes the explore layers functionality.