New explore dropdown UI #2336
New explore dropdown UI #2336
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 ESLint
npm warn config production Use WalkthroughThis 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
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (1)
🔇 Additional comments (10)
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).
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
src/features/projectsV2/ProjectsMap/MapFeatureExplorer/MobileMapSettingsLayout.module.scss (3)
1-11: Consider using CSS custom properties for reusable values.The border radius and max-width values could be defined as CSS custom properties for better maintainability and consistency.
+:root { + --border-radius-large: 12px; + --mobile-settings-width: 293px; +} + .container { display: flex; flex-direction: column; height: 100vh; width: 100%; - max-width: 293px; + max-width: var(--mobile-settings-width); position: absolute; right: 0; top: 0; - border-radius: 12px 0 0 12px; + border-radius: var(--border-radius-large) 0 0 var(--border-radius-large); }
38-53: Add support for Firefox scrollbar styling.The scrollbar is only hidden for WebKit browsers. Consider adding Firefox support using
scrollbar-width..scrollableContent { flex: 1; overflow-y: auto; padding: 12px; background-color: #fff; border-radius: 0; transition: border-radius 0.2s ease; + scrollbar-width: none; &::-webkit-scrollbar { display: none; }
55-70: Add hover and focus states to the close button.The close button lacks hover and focus states for better user interaction feedback.
.closeButton { display: flex; align-items: center; justify-content: center; padding: 4px; cursor: pointer; background: transparent; border: none; + border-radius: 4px; + transition: background-color 0.2s ease; + + &:hover, + &:focus-visible { + background-color: rgba(0, 0, 0, 0.05); + } + + &:focus-visible { + outline: 2px solid #000; + outline-offset: 2px; + } svg { width: 16px; path { fill: currentColor; } } }src/features/projectsV2/ProjectsMap/MapFeatureExplorer/MobileMapSettingsLayout.tsx (2)
20-27: Consider debouncing the scroll handler for better performance.The scroll handler is called on every scroll event, which could impact performance. Consider debouncing the function.
+import { debounce } from 'lodash'; + const MobileMapSettingsLayout = ({ setIsOpen, children }: Props) => { const tMaps = useTranslations('Maps'); const [isAtBottom, setIsAtBottom] = useState(false); const scrollRef = useRef<HTMLDivElement>(null); - const handleScroll = () => { + const handleScroll = debounce(() => { if (!scrollRef.current) return; const { scrollHeight, scrollTop, clientHeight } = scrollRef.current; // Add a small buffer (10px) to trigger the border radius transition a little before the bottom const isBottom = Math.abs(scrollHeight - scrollTop - clientHeight) <= 10; setIsAtBottom(isBottom); - }; + }, 100);
44-64: Add keyboard navigation support.The component should handle keyboard events (e.g., Escape key) for better accessibility.
+ useEffect(() => { + const handleKeyDown = (event: KeyboardEvent) => { + if (event.key === 'Escape') { + setIsOpen(false); + } + }; + + window.addEventListener('keydown', handleKeyDown); + return () => window.removeEventListener('keydown', handleKeyDown); + }, [setIsOpen]); + return ( - <div className={styles.container}> + <div + className={styles.container} + role="dialog" + aria-modal="true" + aria-label={tMaps('explore')} + >
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/features/projectsV2/ProjectsMap/MapFeatureExplorer/MobileMapSettingsLayout.module.scss(1 hunks)src/features/projectsV2/ProjectsMap/MapFeatureExplorer/MobileMapSettingsLayout.tsx(1 hunks)
🔇 Additional comments (1)
src/features/projectsV2/ProjectsMap/MapFeatureExplorer/MobileMapSettingsLayout.tsx (1)
51-53: Add accessibility attributes to the close button.The close button needs proper accessibility attributes for screen readers.
-<button className={styles.closeButton} onClick={() => setIsOpen(false)}> +<button + className={styles.closeButton} + onClick={() => setIsOpen(false)} + aria-label={tMaps('close')} + type="button" +>
…-the-Planet-org/planet-webapp into feature/new-explore-ui
mohitb35
force-pushed
the
feature/new-explore-ui
branch
from
cdf2e65 to
49f386e
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/features/projectsV2/ProjectsMap/MapFeatureExplorer/MapFeatureExplorer.module.scss (2)
52-63:⚠️ Potential issueFix mobile layout overflow handling.
Using
max-height: max-contentbreaks the parent container's scroll behavior.@include xsPhoneView { - max-height: max-content; + max-height: none; }
114-130:⚠️ 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 (2)
src/features/projectsV2/ProjectsMap/MapFeatureExplorer/microComponents/SingleLayerOption.tsx (2)
82-89: Prevent event bubbling in LayerInfoPopupContent.The
onMouseEnterhandler is usingstopPropagation()but the component is still receiving props for mouse events. This could lead to unexpected behavior.- <div onMouseEnter={(e) => e.stopPropagation()}> - <LayerInfoPopupContent - anchorElement={anchor} - setAnchorElement={setAnchor} - additionalInfo={layerConfig.additionalInfo} - handleMouseLeave={handleClose} - /> - </div> + <div> + <LayerInfoPopupContent + anchorElement={anchor} + setAnchorElement={setAnchor} + additionalInfo={layerConfig.additionalInfo} + /> + </div>
11-13: Add Props interface for LayerConfig.The component should define the shape of the
layerConfigprop to ensure type safety.interface Props { - layerConfig: LayerConfig; + layerConfig: { + key: string; + color?: string; + isAvailable: boolean; + isVisible?: boolean; + additionalInfo?: { + label: string; + source: string; + }; + }; + onLayerToggle?: (key: string, isVisible: boolean) => void; }
📜 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/microComponents/SingleLayerOption.tsx(1 hunks)
🔇 Additional comments (2)
src/features/projectsV2/ProjectsMap/MapFeatureExplorer/microComponents/SingleLayerOption.tsx (1)
57-57:⚠️ Potential issueAdd onChange handler to StyledSwitch.
The switch component is missing an onChange handler and checked state, making it non-interactive.
- <StyledSwitch customColor={layerConfig.color} /> + <StyledSwitch + customColor={layerConfig.color} + checked={layerConfig.isVisible} + onChange={(event) => { + // Handle layer visibility toggle + onLayerToggle?.(layerConfig.key, event.target.checked); + }} + />Likely invalid or redundant comment.
src/features/projectsV2/ProjectsMap/MapFeatureExplorer/MapFeatureExplorer.module.scss (1)
42-48:⚠️ Potential issueFix mobile viewport height issues.
Using
100vhcan cause issues with mobile browsers' dynamic toolbars.max-height: 100vh; - height: 100vh; + height: 100vh; /* Fallback */ + height: -webkit-fill-available; + height: stretch; overflow-y: scroll; min-width: 293px;Likely invalid or redundant comment.
- also refactors MapOptions (ProjectsMapContext) to use layer names as keys - refactors code that uses MapOptions
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (11)
src/features/projectsV2/ProjectsMap/MapFeatureExplorer/microComponents/MapSettingsSection.tsx (2)
14-14: Consider using string literal union type for groupKey.The
groupKeytype could be more maintainable by defining it as a separate type or enum.- groupKey: 'soil' | 'forests' | 'biodiversity' | 'risks'; +type LayerGroup = 'soil' | 'forests' | 'biodiversity' | 'risks'; + groupKey: LayerGroup;
41-43: Improve empty fragment handling.The empty fragment for unavailable layers could be optimized.
- {config.map((layerConfig) => { - if (!layerConfig.isAvailable) return <></>; - return ( + {config.filter(layerConfig => layerConfig.isAvailable).map((layerConfig) => (src/features/projectsV2/ProjectsMap/MapFeatureExplorer/microComponents/SingleLayerOption.tsx (3)
40-47: Consider using Material-UI's built-in hover handling.The manual mouse event handling could be simplified by using Material-UI's built-in hover handling with the
HoverPopovervariant.- const handleMouseLeave = useCallback((e: MouseEvent) => { - const relatedTarget = e.relatedTarget as HTMLElement; - const isMovingToPopover = relatedTarget?.closest('[role="presentation"]'); - - if (!isMovingToPopover) { - handleClose(); - } - }, []);Replace with Material-UI's HoverPopover:
<Popover open={Boolean(anchor)} onClose={handleClose} anchorEl={anchor} variant="hover" // ... other props >
95-102: Simplify event handling in popover content.The nested
onMouseEnterwithstopPropagationcould be removed as it's not necessary with Material-UI's popover.- <div onMouseEnter={(e) => e.stopPropagation()}> + <div> <LayerInfoPopupContent anchorElement={anchor} setAnchorElement={setAnchor} additionalInfo={layerConfig.additionalInfo} - handleMouseLeave={handleClose} /> </div>
87-93: Consider extracting popover styles to a theme or styles file.The inline styles for the popover could be moved to a separate styles file for better maintainability.
// MapFeatureExplorer.module.scss .popoverPaper { border-radius: 12px; margin-top: -8px; } // Component <Popover sx={{ '& .MuiPaper-root': styles.popoverPaper }} // ... other props >src/features/projectsV2/ProjectsMap/index.tsx (2)
131-134: Consider extracting complex conditions into named variables.The multiple projects view condition could be more readable if broken down into named parts.
+ const hasProjects = Boolean(mapOptions.projects) && projects && projects.length > 0; const shouldShowMultipleProjectsView = - Boolean(mapOptions.projects) && - projects && - projects.length > 0 && - !shouldShowSingleProjectsView; + hasProjects && !shouldShowSingleProjectsView;
Line range hint
135-143: Consider simplifying navigation controls visibility logic.The conditions for showing navigation controls could be simplified by combining the checks.
- const shouldShowMultiPlantLocationInfo = - props.isMobile && - selectedSamplePlantLocation === null && - selectedPlantLocation?.type === 'multi-tree-registration'; - const shouldShowSinglePlantLocationInfo = - props.isMobile && - (selectedSamplePlantLocation !== null || - selectedPlantLocation?.type === 'single-tree-registration'); - const shouldShowNavigationControls = !( - shouldShowMultiPlantLocationInfo || shouldShowSinglePlantLocationInfo - ); + const isShowingPlantLocationInfo = props.isMobile && ( + (selectedSamplePlantLocation === null && selectedPlantLocation?.type === 'multi-tree-registration') || + selectedSamplePlantLocation !== null || + selectedPlantLocation?.type === 'single-tree-registration' + ); + const shouldShowNavigationControls = !isShowingPlantLocationInfo;src/utils/mapsV2/mapSettings.config.ts (4)
1-19: Consider adding JSDoc comments for better type documentation.The
MapLayerOptionsTypeunion type is well-structured and logically grouped. Consider adding JSDoc comments to document the purpose and usage of this type.+/** + * Represents the available map layer options that can be toggled in the explore dropdown. + * Options are grouped into categories: projects, forests, soil, biodiversity, and risks. + */ export type MapLayerOptionsType = | 'projects' | 'forestCover' // ... rest of the type definition
20-44: LGTM! Consider type enhancements for better type safety.The interface hierarchy is well-designed. Consider these type enhancements:
export interface AdditionalInfo { resolution: string; - dataYears: string; + dataYears: `${number}` | `${number}-${number}`; // More specific type for year formats description: string; underlyingData: string; source: { text: string; - url: string; + url: `https://${string}`; // Ensure URLs are HTTPS }; } export interface LayerConfig { key: MapLayerOptionsType; isAvailable: boolean; additionalInfo?: AdditionalInfo; - color?: string; + color?: `#${string}`; // Ensure hex color format }
140-141: Fix typos and standardize unit formats.There are several typos and inconsistencies in the descriptions:
- "horrizon" should be "horizon"
- Units are inconsistently formatted: "cg/kg", "dg/kg", "cg/cm3; cg/kg"
- 'cg/kg; 0-30 cm horrizon (A weighted average for all depths)', + 'cg/kg; 0-30 cm horizon (A weighted average for all depths)', - 'dg/kg; 0-30 cm horrizon (A weighted average for all depths)', + 'dg/kg; 0-30 cm horizon (A weighted average for all depths)', - 'cg/cm3; cg/kg; 0-30 cm horrizon (A weighted average for all depths)', + 'cg/cm³; 0-30 cm horizon (A weighted average for all depths)',Also applies to: 171-172, 187-188
57-57: Standardize resolution formats.The resolution values use inconsistent formats and units:
- Some use "~" prefix while others don't
- Units vary between m, km, and km²
Consider standardizing the resolution format to always include:
- The "~" prefix
- A space between the number and unit
- Consistent unit notation (m, km)
Example:
- resolution: '~500m (Downscaled)', + resolution: '~500 m (Downscaled)', - resolution: '~500km', + resolution: '~500 km',Also applies to: 73-73, 88-88, 105-105, 120-120, 137-137, 153-153, 168-168, 184-184, 202-202, 218-218, 234-234, 249-249, 266-266, 280-280
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
src/features/common/Layout/ProjectsLayout/index.tsx(1 hunks)src/features/projectsV2/ProjectsMap/MapFeatureExplorer/MapSettings.tsx(1 hunks)src/features/projectsV2/ProjectsMap/MapFeatureExplorer/microComponents/MapSettingsSection.tsx(1 hunks)src/features/projectsV2/ProjectsMap/MapFeatureExplorer/microComponents/SingleLayerOption.tsx(1 hunks)src/features/projectsV2/ProjectsMap/index.tsx(1 hunks)src/features/projectsV2/ProjectsMap/stories/MapFeatureExplorer.stories.tsx(1 hunks)src/features/projectsV2/ProjectsMapContext.tsx(3 hunks)src/utils/mapsV2/mapSettings.config.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/features/projectsV2/ProjectsMap/MapFeatureExplorer/MapSettings.tsx
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (javascript)
🔇 Additional comments (5)
src/features/projectsV2/ProjectsMap/MapFeatureExplorer/microComponents/MapSettingsSection.tsx (1)
62-62: Remove commented-out code.The commented-out code should be removed to improve code maintainability.
src/features/projectsV2/ProjectsMap/stories/MapFeatureExplorer.stories.tsx (1)
17-17: LGTM!The state property rename from
showProjectstoprojectsaligns with the broader refactoring ofmapOptionsacross the codebase.src/features/common/Layout/ProjectsLayout/index.tsx (1)
32-32: LGTM!The
Boolean()wrapper ensures type safety when checkingmapOptions.projects, which is good practice since it's now an optional property.src/features/projectsV2/ProjectsMapContext.tsx (2)
50-54: Great improvement to type flexibility!The new
MapOptionstype using mapped types ([key in MapLayerOptionsType]) makes the context more extensible and type-safe. This will make it easier to add new map layer options in the future.
81-81: LGTM!The initial state update aligns with the new type definition while maintaining the same default behavior.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
src/features/projectsV2/ProjectsMap/MapFeatureExplorer/YearSlider/index.tsx (1)
1-2: Remove or update the commented code status.Since this component is now being actively used for the explore dropdown UI, the comment indicating it's unused should be removed or updated to reflect its current status.
-// Unused code - for future reference while implementing the deforestation timeline slidersrc/features/projectsV2/ProjectsMap/MapFeatureExplorer/YearSlider/YearSlider.module.scss (2)
3-9: Consider responsive dimensions for mobile view.The main container has fixed width and height values. Consider using relative units or media queries for better mobile responsiveness.
.rangeMainContainer { - width: 150px; + width: min(150px, 100%); height: 30px; background-color: rgba(var(--deforestration-range-background-new), 0.1); margin-top: 6px; border-radius: 6px; + @media (max-width: 768px) { + width: 100%; + } }
22-26: Adjust slider container width for mobile view.The slider container has a fixed width which might not work well on smaller screens.
.sliderContainer { - width: 107px; + width: min(107px, calc(100% - 43px)); /* Subtracting space for play icon and gaps */ display: flex; margin-top: 10px; + @media (max-width: 768px) { + width: calc(100% - 43px); + } }src/features/projectsV2/ProjectsMap/MapFeatureExplorer/MapFeatureExplorer.module.scss (1)
79-112: Consider increasing text-underline-offset for better readability.The current underline offset might be too close to the text. Consider increasing it slightly:
- text-underline-offset: 2px; + text-underline-offset: 3px;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/features/projectsV2/ProjectsMap/MapFeatureExplorer/MapFeatureExplorer.module.scss(2 hunks)src/features/projectsV2/ProjectsMap/MapFeatureExplorer/YearSlider/YearSlider.module.scss(1 hunks)src/features/projectsV2/ProjectsMap/MapFeatureExplorer/YearSlider/index.tsx(1 hunks)src/features/projectsV2/ProjectsMap/MapFeatureExplorer/index.tsx(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/features/projectsV2/ProjectsMap/MapFeatureExplorer/index.tsx
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (javascript)
🔇 Additional comments (7)
src/features/projectsV2/ProjectsMap/MapFeatureExplorer/MapFeatureExplorer.module.scss (7)
10-25: LGTM! Well-structured button content styles.The implementation follows best practices with:
- Proper BEM naming
- Responsive font sizes using variables
- Mobile-first approach with breakpoints
42-48: Mobile viewport height needs fallbacks.The use of
100vhcan cause issues with mobile browsers' dynamic toolbars. Consider using-webkit-fill-availableor a more robust solution.
56-58: Consider accessibility implications of hiding scrollbars.Hiding scrollbars can impact accessibility. Consider using thin scrollbars instead.
60-62: Review mobile layout overflow handling.Using
max-height: max-contentbreaks the parent container's scroll behavior.
65-77: LGTM! Well-structured section styles.Good use of semantic markup and consistent styling with variables.
114-130: Address accessibility concerns in the info popup.The implementation has potential accessibility issues with font size and fixed width.
Line range hint
134-162: LGTM! Consistent styling updates.Good use of CSS variables and consistent styling patterns.
src/features/projectsV2/ProjectsMap/MapFeatureExplorer/YearSlider/index.tsx
Show resolved
Hide resolved
src/features/projectsV2/ProjectsMap/MapFeatureExplorer/YearSlider/index.tsx
Show resolved
Hide resolved
src/features/projectsV2/ProjectsMap/MapFeatureExplorer/YearSlider/index.tsx
Show resolved
Hide resolved
src/features/projectsV2/ProjectsMap/MapFeatureExplorer/MapFeatureExplorer.module.scss
Show resolved
Hide resolved
mohitb35
force-pushed
the
feature/new-explore-ui
branch
from
9f337bd to
ff6494b
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
src/features/projectsV2/ProjectsMap/MapFeatureExplorer/MapFeatureExplorer.module.scss (2)
65-76: Consider using a color variable for consistency.The rgba color value could be moved to a variable in the theme file for better maintainability and consistency.
+// In theme file +$exploreItemBackground: rgba(0, 122, 73, 0.05); // In this file .exploreItemSection { - background: rgba(0, 122, 73, 0.05); + background: $exploreItemBackground; border-radius: 8px; padding: 8px 9px; font-size: 12px; margin-bottom: 10px; }
Line range hint
1-133: Consider organizing mobile styles using a mobile-first approach.The current implementation scatters mobile-specific styles across different classes. Consider adopting a mobile-first approach by:
- Defining base styles for mobile
- Using media queries to enhance for larger screens
Example reorganization:
.exploreMainContainer { // Mobile base styles position: absolute; max-height: 100vh; height: 100vh; overflow-y: scroll; min-width: 293px; border-radius: 12px 0 0 12px; right: 0px; margin-top: 0; // Desktop enhancements @include desktop { position: static; max-width: 240px; max-height: 485px; margin-top: 10px; } }src/features/projectsV2/ProjectsMap/MapFeatureExplorer/index.tsx (1)
43-60: Enhance Modal accessibility.The Modal implementation could benefit from additional accessibility features.
Consider these improvements:
<Modal open={isOpen} aria-labelledby="map-settings-menu" + aria-describedby="map-settings-description" onClose={(_event, reason) => { if (reason === 'backdropClick') { setIsOpen(false); } }} + onKeyDown={(e) => { + if (e.key === 'Escape') { + setIsOpen(false); + } + }} >src/features/projectsV2/ProjectsMap/MapFeatureExplorer/MapSettings.tsx (1)
26-59: Consider extracting map settings configuration to a constant.The repeated MapSettingsSection components could be generated from a configuration array.
Consider this refactor:
+const SECTION_CONFIGS = [ + { groupKey: undefined, config: mapSettingsConfig.projects }, + { groupKey: 'forests', config: mapSettingsConfig.forests }, + { groupKey: 'soil', config: mapSettingsConfig.soil }, + { groupKey: 'biodiversity', config: mapSettingsConfig.biodiversity }, + { groupKey: 'risks', config: mapSettingsConfig.risks }, +]; const content = ( <> - <MapSettingsSection - config={mapSettingsConfig.projects} - mapOptions={mapOptions} - updateMapOption={updateMapOption} - /> - // ... other sections + {SECTION_CONFIGS.map(({ groupKey, config }) => ( + <MapSettingsSection + key={groupKey || 'projects'} + groupKey={groupKey} + config={config} + mapOptions={mapOptions} + updateMapOption={updateMapOption} + /> + ))} <div className={styles.exploreDescription}>{tMaps('3trilliontrees')}</div> </> );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/features/projectsV2/ProjectsMap/MapFeatureExplorer/MapFeatureExplorer.module.scss(1 hunks)src/features/projectsV2/ProjectsMap/MapFeatureExplorer/MapSettings.tsx(1 hunks)src/features/projectsV2/ProjectsMap/MapFeatureExplorer/YearSlider/CustomSlider.tsx(1 hunks)src/features/projectsV2/ProjectsMap/MapFeatureExplorer/YearSlider/index.tsx(1 hunks)src/features/projectsV2/ProjectsMap/MapFeatureExplorer/index.tsx(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/features/projectsV2/ProjectsMap/MapFeatureExplorer/YearSlider/CustomSlider.tsx
🧰 Additional context used
📓 Learnings (1)
src/features/projectsV2/ProjectsMap/MapFeatureExplorer/YearSlider/index.tsx (3)
Learnt from: mohitb35
PR: Plant-for-the-Planet-org/planet-webapp#2336
File: src/features/projectsV2/ProjectsMap/MapFeatureExplorer/YearSlider/index.tsx:54-55
Timestamp: 2025-01-31T10:02:39.103Z
Learning: The YearRangeSlider component in src/features/projectsV2/ProjectsMap/MapFeatureExplorer/YearSlider/index.tsx is a reference implementation for future use in the deforestation timeline feature and is not currently in use.
Learnt from: mohitb35
PR: Plant-for-the-Planet-org/planet-webapp#2336
File: src/features/projectsV2/ProjectsMap/MapFeatureExplorer/YearSlider/index.tsx:8-11
Timestamp: 2025-01-31T10:03:19.293Z
Learning: The YearRangeSlider component in src/features/projectsV2/ProjectsMap/MapFeatureExplorer/YearSlider/index.tsx is a reference implementation for a future deforestation timeline slider feature and is not currently in use.
Learnt from: mohitb35
PR: Plant-for-the-Planet-org/planet-webapp#2336
File: src/features/projectsV2/ProjectsMap/MapFeatureExplorer/YearSlider/index.tsx:17-19
Timestamp: 2025-01-31T10:02:57.320Z
Learning: Code marked with comments like "Unused code" or "for future reference" should not be reviewed as it's not meant for immediate use.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (javascript)
🔇 Additional comments (9)
src/features/projectsV2/ProjectsMap/MapFeatureExplorer/MapFeatureExplorer.module.scss (3)
10-25: LGTM! Clean implementation of responsive button content.The flex layout and responsive font sizing are well implemented.
42-48: Mobile viewport height needs fallbacks.As previously discussed, using
100vhcan cause issues with mobile browsers' dynamic toolbars. The suggested fallbacks should be implemented.
60-62: Fix mobile layout overflow handling.Using
max-height: max-contentbreaks the parent container's scroll behavior. The container should inherit its parent's scrollable area.src/features/projectsV2/ProjectsMap/MapFeatureExplorer/YearSlider/index.tsx (1)
1-59: Acknowledged: Code reserved for future implementation.This file contains reference code for a future deforestation timeline slider feature and is not currently in use.
src/features/projectsV2/ProjectsMap/MapFeatureExplorer/index.tsx (3)
14-14: LGTM: isMobile prop addition.The optional isMobile prop is correctly typed and integrated into the component props.
31-34: LGTM: Responsive button content.Good implementation of conditional rendering for the subtitle based on device type.
37-42: LGTM: Desktop view implementation.Clean implementation of conditional rendering for desktop view.
src/features/projectsV2/ProjectsMap/MapFeatureExplorer/MapSettings.tsx (2)
14-15: LGTM: Props type definition.The optional isMobile and setIsOpen props are correctly typed using SetState type.
61-67: LGTM: Mobile layout handling.Good implementation of conditional rendering for mobile layout using MobileMapSettingsLayout component.
This PR handles the new explore drop down UI (mobile, desktop).