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 comprehensive mobile-responsive enhancements to the Map Feature Explorer in the Projects V2 section. The changes include creating new components like Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ 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.
| const MapLayerControlPanel = ({ | ||
| category, | ||
| exploreConfig, | ||
| mapOptions, | ||
| updateMapOption, | ||
| }: Props) => { | ||
| return ( | ||
| <section className={styles.exploreItemSection}> | ||
| {category && <h2>{category}</h2>} | ||
| <div> | ||
| {exploreConfig.map((item) => { | ||
| if (!item.shouldRender) return <></>; | ||
| return ( | ||
| <LayerSwitchContainer | ||
| key={item.label} | ||
| showDivider={item.showDivider} | ||
| switchComponent={ | ||
| <StyledSwitch | ||
| checked={mapOptions?.['showProjects'] || false} | ||
| customColor={item.color} | ||
| onChange={( | ||
| _event: ChangeEvent<HTMLInputElement>, | ||
| checked: boolean | ||
| ) => { | ||
| if (updateMapOption) | ||
| updateMapOption('showProjects', checked); | ||
| }} | ||
| /> | ||
| } | ||
| label={item.label} | ||
| additionalInfo={item.additionalInfo} | ||
| /> | ||
| ); | ||
| })} | ||
| </div> | ||
| </section> | ||
| ); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Proposed switch logic improvements
Here, each switch is mapped to 'showProjects'. If you plan on handling different map options, pass a dynamic key from each item object so each item toggles the correct option.
- updateMapOption('showProjects', checked);
+ if (item.mapOptionKey) {
+ updateMapOption(item.mapOptionKey, checked);
+ }Committable suggestion skipped: line range outside the PR's diff.
| const LayerInfoPopupContent = ({ | ||
| additionalInfo, | ||
| handleMouseLeave, | ||
| setAnchorEl, | ||
| anchorEl, | ||
| }: Props) => { | ||
| const tMaps = useTranslations('Maps'); | ||
| return ( | ||
| <div | ||
| className={styles.layerInfoPopupContainer} | ||
| onMouseEnter={() => setAnchorEl(anchorEl)} | ||
| onMouseLeave={handleMouseLeave} | ||
| > | ||
| {additionalInfo?.dataYears && ( | ||
| <div> | ||
| <p className={styles.label}>{tMaps('layers.dataYears')}</p> | ||
| <p>{additionalInfo?.dataYears}</p> | ||
| </div> | ||
| )} | ||
| {additionalInfo?.resolution && ( | ||
| <div> | ||
| <p className={styles.label}>{tMaps('layers.resolution')}</p> | ||
| <p>~{additionalInfo?.resolution}</p> | ||
| </div> | ||
| )} | ||
| {additionalInfo?.description && ( | ||
| <div> | ||
| <p className={styles.label}>{tMaps('layers.description')}</p> | ||
| <p>{additionalInfo.description}</p> | ||
| </div> | ||
| )} | ||
| {additionalInfo?.underlyingData && ( | ||
| <div> | ||
| <p className={styles.label}>{tMaps('layers.underlyingData')}</p> | ||
| <p>{additionalInfo.underlyingData}</p> | ||
| </div> | ||
| )} | ||
| {additionalInfo?.covariates && ( | ||
| <div> | ||
| <p>{tMaps('layers.covariates')}</p> | ||
| <a | ||
| href={additionalInfo.source} | ||
| target="_blank" | ||
| rel="noopener noreferrer" | ||
| className={styles.source} | ||
| > | ||
| {additionalInfo.covariates} | ||
| </a> | ||
| </div> | ||
| )} | ||
| </div> | ||
| ); | ||
| }; |
There was a problem hiding this comment.
Validate link fields before rendering
When rendering href from additionalInfo.source, consider validating or sanitizing the input to prevent potential security risks (e.g., ensuring it uses a safe protocol).
<a
- href={additionalInfo.source}
+ href={additionalInfo.source?.startsWith('http') ? additionalInfo.source : '#'}
target="_blank"
rel="noopener noreferrer"
className={styles.source}
>| {isMobile && setIsOpen && ( | ||
| <ExploreDropdownHeaderMobile setIsOpen={setIsOpen} /> | ||
| )} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid cramped mobile headers.
ExploreDropdownHeaderMobile typically runs close to the top of the viewport. Ensure there's enough breathing room or margins in the enclosing layout so the header doesn't visually clash with the device notch or other UI elements.
|
| .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).
| min-width: 293px; | ||
| border-radius: unset; | ||
| right: 0px; | ||
| top: -10px; |
There was a problem hiding this comment.
Why do this instead of making margin-top = 0px? This makes it tougher to understand how the element is positioned.
| const MapLayerControlPanel = ({ | ||
| category, |
There was a problem hiding this comment.
I feel it may be more appropriate to name this MapSettingsSection as it conveys the purpose of this component better (grouping).
|
I think we should stretch the menu from top to bottom of the screen. We should also accomodate for scroll, that is currently not possible. |
- scrollable full height layout with fixed header - removes now unused ExploreDropdownHeaderMobile component - refactors MapSettings to use MobileMapSettingsLayout, separating menu content - set up temporary config/duplicate layers for scrolling demo - minor style changes to exploreMainContainer on mobile
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
src/features/projectsV2/ProjectsMap/MapFeatureExplorer/MapSettings.tsx (2)
6-8:⚠️ Potential issueRemove commented-out code.
As suggested in the PR comments, remove all commented-out code to improve maintainability.
Also applies to: 137-171
172-204:⚠️ Potential issueRemove duplicate MapLayerControlPanel components in desktop view.
The same MapLayerControlPanel components are duplicated in the desktop view as well.
🧹 Nitpick comments (4)
src/features/projectsV2/ProjectsMap/MapFeatureExplorer/MobileMapSettingsLayout.tsx (1)
9-12: Add JSDoc comments for Props interface.Add documentation for the Props interface to improve code maintainability.
+/** + * Props for the MobileMapSettingsLayout component + * @property {SetState<boolean>} setIsOpen - Function to control the visibility of the layout + * @property {ReactNode} children - Content to be rendered within the layout + */ interface Props { setIsOpen: SetState<boolean>; children: ReactNode; }src/features/projectsV2/ProjectsMap/MapFeatureExplorer/MapFeatureExplorer.module.scss (2)
12-12: Use theme variable for background color.Replace hardcoded color with theme variable for consistency.
- background-color: #fff; + background-color: $backgroundColor;
19-21: Consolidate scrollbar styles.The scrollbar hiding styles are duplicated. Consider creating a mixin for reusability.
+@mixin hide-scrollbar { + &::-webkit-scrollbar { + display: none; + } + -ms-overflow-style: none; + scrollbar-width: none; +} .exploreMainContainer { - &::-webkit-scrollbar { - display: none; - } + @include hide-scrollbar; } .exploreItemsContainer { - &::-webkit-scrollbar { - display: none; - } + @include hide-scrollbar; }Also applies to: 39-41
src/features/projectsV2/ProjectsMap/MapFeatureExplorer/MapSettings.tsx (1)
31-77: Move configuration to a separate file.The
forestConfigarray should be moved to a separate configuration file for better maintainability.Create a new file
src/features/projectsV2/ProjectsMap/MapFeatureExplorer/config/forestLayers.ts:import { MapLayerConfig } from '../types'; export const getForestConfig = (tMaps: any, primaryColorNew: string): MapLayerConfig[] => [ { label: tMaps('layers.forestLayers.canopyHeight'), color: undefined, showDivider: true, shouldRender: true, additionalInfo: { // ... rest of the config }, }, // ... rest of the layers ];
📜 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/MapSettings.tsx(3 hunks)src/features/projectsV2/ProjectsMap/MapFeatureExplorer/MobileMapSettingsLayout.module.scss(1 hunks)src/features/projectsV2/ProjectsMap/MapFeatureExplorer/MobileMapSettingsLayout.tsx(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/features/projectsV2/ProjectsMap/MapFeatureExplorer/MobileMapSettingsLayout.module.scss
🔇 Additional comments (1)
src/features/projectsV2/ProjectsMap/MapFeatureExplorer/MapFeatureExplorer.module.scss (1)
25-31: Review mobile height calculation.The current implementation using
height: 100vhmight cause issues with mobile browsers' dynamic toolbars. Consider using-webkit-fill-availablefor better mobile support.@include xsPhoneView { max-height: 100vh; - height: 100vh; + height: 100vh; + height: -webkit-fill-available; overflow-y: scroll; min-width: 293px; border-radius: 12px 0 0 12px; right: 0px;✅ Verification successful
Consider mobile viewport height fix across the codebase
The mobile height calculation issue with
100vhis valid and appears in multiple components. The proposed-webkit-fill-availablesolution should be implemented, and similar fixes should be considered for other mobile views using viewport height units.
- Current file:
src/features/projectsV2/ProjectsMap/MapFeatureExplorer/MapFeatureExplorer.module.scss- Other affected files:
src/features/projectsV2/ProjectsMap/MobileMapSettingsLayout.module.scsssrc/features/user/CompleteSignup/CompleteSignup.module.scss- Other mobile-first components using
100vh🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash # Search for viewport height related patterns in SCSS files echo "=== Searching for viewport height usage ===" rg "height:\s*\d+vh" -t scss echo -e "\n=== Searching for -webkit-fill-available usage ===" rg "webkit-fill-available" -t scss echo -e "\n=== Searching for mobile-specific height handling ===" rg "@include.*phone.*height" -t scssLength of output: 528
Script:
#!/bin/bash # Search for viewport height related patterns echo "=== Searching for viewport height usage ===" rg "height:\s*\d+vh" --glob "*.{scss,sass}" echo -e "\n=== Searching for -webkit-fill-available usage ===" rg "webkit-fill-available" --glob "*.{scss,sass}" echo -e "\n=== Searching for mobile-specific height handling ===" rg "(@include.*phone|@media.*max-width).*height" --glob "*.{scss,sass}" echo -e "\n=== Searching for min-height viewport units ===" rg "min-height:\s*\d+vh" --glob "*.{scss,sass}"Length of output: 4161
| <button className={styles.closeButton} onClick={() => setIsOpen(false)}> | ||
| <CrossIcon /> | ||
| </button> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
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"
+>Committable suggestion skipped: line range outside the PR's diff.
| <MapLayerControlPanel | ||
| exploreConfig={projectConfig} | ||
| updateMapOption={updateMapOption} | ||
| mapOptions={mapOptions} | ||
| /> | ||
| <MapLayerControlPanel | ||
| category={tMaps('layers.forests')} | ||
| exploreConfig={forestConfig} | ||
| /> | ||
| <MapLayerControlPanel | ||
| category={tMaps('layers.forests')} | ||
| exploreConfig={forestConfig} | ||
| /> | ||
| <MapLayerControlPanel | ||
| category={tMaps('layers.forests')} | ||
| exploreConfig={forestConfig} | ||
| /> | ||
| <MapLayerControlPanel | ||
| category={tMaps('layers.forests')} | ||
| exploreConfig={forestConfig} | ||
| /> | ||
| <MapLayerControlPanel | ||
| category={tMaps('layers.forests')} | ||
| exploreConfig={forestConfig} | ||
| /> | ||
| <MapLayerControlPanel | ||
| category={tMaps('layers.forests')} | ||
| exploreConfig={forestConfig} | ||
| /> | ||
| <MapLayerControlPanel | ||
| category={tMaps('layers.forests')} | ||
| exploreConfig={forestConfig} | ||
| /> |
There was a problem hiding this comment.
Refactor duplicate MapLayerControlPanel components.
Multiple identical MapLayerControlPanel components are being rendered. Consider using a loop or reducing the number of components if they're truly needed.
- <MapLayerControlPanel
- category={tMaps('layers.forests')}
- exploreConfig={forestConfig}
- />
- // ... repeated 6 more times
+ {Array(7).fill(null).map((_, index) => (
+ <MapLayerControlPanel
+ key={`forest-layer-${index}`}
+ category={tMaps('layers.forests')}
+ exploreConfig={forestConfig}
+ />
+ ))}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <MapLayerControlPanel | |
| exploreConfig={projectConfig} | |
| updateMapOption={updateMapOption} | |
| mapOptions={mapOptions} | |
| /> | |
| <MapLayerControlPanel | |
| category={tMaps('layers.forests')} | |
| exploreConfig={forestConfig} | |
| /> | |
| <MapLayerControlPanel | |
| category={tMaps('layers.forests')} | |
| exploreConfig={forestConfig} | |
| /> | |
| <MapLayerControlPanel | |
| category={tMaps('layers.forests')} | |
| exploreConfig={forestConfig} | |
| /> | |
| <MapLayerControlPanel | |
| category={tMaps('layers.forests')} | |
| exploreConfig={forestConfig} | |
| /> | |
| <MapLayerControlPanel | |
| category={tMaps('layers.forests')} | |
| exploreConfig={forestConfig} | |
| /> | |
| <MapLayerControlPanel | |
| category={tMaps('layers.forests')} | |
| exploreConfig={forestConfig} | |
| /> | |
| <MapLayerControlPanel | |
| category={tMaps('layers.forests')} | |
| exploreConfig={forestConfig} | |
| /> | |
| <MapLayerControlPanel | |
| exploreConfig={projectConfig} | |
| updateMapOption={updateMapOption} | |
| mapOptions={mapOptions} | |
| /> | |
| {Array(7).fill(null).map((_, index) => ( | |
| <MapLayerControlPanel | |
| key={`forest-layer-${index}`} | |
| category={tMaps('layers.forests')} | |
| exploreConfig={forestConfig} | |
| /> | |
| ))} |
- refactors code to render single layer or grouped layer options as per the config - updates styles for section - rename LayerSwitchContainer to SingleLayerOption - remove un-required config code
There was a problem hiding this comment.
Actionable comments posted: 7
🔭 Outside diff range comments (1)
src/features/projectsV2/ProjectsMap/MapFeatureExplorer/MapSettings.tsx (1)
Line range hint
80-114: Remove commented MapLayerToggle implementations.Clean up the commented code to improve maintainability.
- {/* <MapLayerToggle - label={tAllProjects('currentForests')} - switchComponent={ - <StyledSwitch customColor={`${primaryColorNew}`} /> - } - /> */} - {/* <div className={styles.hrLine} /> */} - ... more commented code ...🧰 Tools
🪛 GitHub Check: CodeFactor
[notice] 11-11: src/features/projectsV2/ProjectsMap/MapFeatureExplorer/MapSettings.tsx#L11
'themeProperties' is defined but never used. Allowed unused vars must match /^_/u. (@typescript-eslint/no-unused-vars)
[notice] 24-24: src/features/projectsV2/ProjectsMap/MapFeatureExplorer/MapSettings.tsx#L24
'mapOptions' is defined but never used. Allowed unused args must match /^_/u. (@typescript-eslint/no-unused-vars)
[notice] 25-25: src/features/projectsV2/ProjectsMap/MapFeatureExplorer/MapSettings.tsx#L25
'updateMapOption' is defined but never used. Allowed unused args must match /^_/u. (@typescript-eslint/no-unused-vars)
🧹 Nitpick comments (7)
src/utils/mapsV2/mapSettings.config.ts (2)
140-140: Fix typo in soil layer descriptions.The word "horrizon" is misspelled in multiple soil layer descriptions. It should be "horizon".
- 'cg/kg; 0-30 cm horrizon (A weighted average for all depths)', + 'cg/kg; 0-30 cm horizon (A weighted average for all depths)',Also applies to: 155-155, 171-171, 187-187
57-57: Standardize resolution format across layers.Resolution values use inconsistent units and formats:
- "~500m (Downscaled)"
- "~500km"
- "~1m"
- "~250m"
- "~3km"
- "~10km"
- "~55km"
Consider standardizing to a consistent format and unit (e.g., all in meters or kilometers with consistent notation).
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
src/features/projectsV2/ProjectsMap/MapFeatureExplorer/microComponents/SingleLayerOption.tsx (1)
1-2: Clean up unused imports.Remove the following unused imports to improve code maintainability:
AdditionalInfoChangeEvent,ReactNodeuseStateMapOptions- Duplicate import of
PopoverAlso applies to: 5-5, 11-11, 13-14
🧰 Tools
🪛 GitHub Check: CodeFactor
[notice] 1-1: src/features/projectsV2/ProjectsMap/MapFeatureExplorer/microComponents/SingleLayerOption.tsx#L1
'AdditionalInfo' is defined but never used. Allowed unused vars must match /^_/u. (@typescript-eslint/no-unused-vars)
[notice] 2-2: src/features/projectsV2/ProjectsMap/MapFeatureExplorer/microComponents/SingleLayerOption.tsx#L2
'ReactNode' is defined but never used. Allowed unused vars must match /^_/u. (@typescript-eslint/no-unused-vars)
[notice] 2-2: src/features/projectsV2/ProjectsMap/MapFeatureExplorer/microComponents/SingleLayerOption.tsx#L2
'ChangeEvent' is defined but never used. Allowed unused vars must match /^_/u. (@typescript-eslint/no-unused-vars)src/features/projectsV2/ProjectsMap/MapFeatureExplorer/microComponents/MapSettingsSection.tsx (2)
1-1: Clean up unused imports.Remove the following unused imports to improve code maintainability:
ChangeEventMapSettingsConfigMapOptionsStyledSwitchAlso applies to: 4-4, 6-6, 8-8
🧰 Tools
🪛 GitHub Check: CodeFactor
[notice] 1-1: src/features/projectsV2/ProjectsMap/MapFeatureExplorer/microComponents/MapSettingsSection.tsx#L1
'ChangeEvent' is defined but never used. Allowed unused vars must match /^_/u. (@typescript-eslint/no-unused-vars)
54-56: Improve handling of unavailable layers.Instead of rendering an empty fragment for unavailable layers, consider filtering them out at the map level to avoid unnecessary component creation.
- return ( - <div> - {config.map((item) => { - if (!item.isAvailable) return <></>; - return <SingleLayerOption key={item.key} layerConfig={item} />; - })} - </div> - ); + return ( + <div> + {config + .filter((item) => item.isAvailable) + .map((item) => ( + <SingleLayerOption key={item.key} layerConfig={item} /> + ))} + </div> + );src/features/projectsV2/ProjectsMap/MapFeatureExplorer/MapFeatureExplorer.module.scss (2)
24-31: Consider mobile viewport and scrolling behavior improvements.The mobile view implementation could benefit from these enhancements:
- Use
dvh(dynamic viewport height) instead ofvhto handle mobile browsers' dynamic toolbars- Add
z-indexto ensure proper layering- Consider adding
overscroll-behaviorto prevent scroll chaining@include xsPhoneView { position: absolute; - max-height: 100vh; - height: 100vh; + max-height: 100dvh; + height: 100dvh; overflow-y: scroll; + overscroll-behavior: contain; min-width: 293px; border-radius: 12px 0 0 12px; right: 0px; + z-index: 1000; margin-top: 0; }
35-46: Improve scrollbar accessibility across browsers.The current implementation hides scrollbars which might affect usability. Consider using a more cross-browser compatible approach that maintains scrollbar visibility while styling it consistently.
.exploreItemsContainer { max-height: 400px; overflow-y: auto; - &::-webkit-scrollbar { - display: none; - } + scrollbar-width: thin; + scrollbar-color: rgba(0, 0, 0, 0.2) transparent; + + &::-webkit-scrollbar { + width: 6px; + } + + &::-webkit-scrollbar-track { + background: transparent; + } + + &::-webkit-scrollbar-thumb { + background-color: rgba(0, 0, 0, 0.2); + border-radius: 3px; + } @include xsPhoneView { max-height: max-content; } }
📜 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 (7)
src/features/projectsV2/ProjectsMap/MapFeatureExplorer/CustomSwitch.tsx(2 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/microComponents/LayerInfoPopupContent.tsx(1 hunks)src/features/projectsV2/ProjectsMap/MapFeatureExplorer/microComponents/MapSettingsSection.tsx(1 hunks)src/features/projectsV2/ProjectsMap/MapFeatureExplorer/microComponents/SingleLayerOption.tsx(1 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/microComponents/LayerInfoPopupContent.tsx
🧰 Additional context used
🪛 GitHub Check: CodeFactor
src/features/projectsV2/ProjectsMap/MapFeatureExplorer/microComponents/SingleLayerOption.tsx
[notice] 1-1: src/features/projectsV2/ProjectsMap/MapFeatureExplorer/microComponents/SingleLayerOption.tsx#L1
'AdditionalInfo' is defined but never used. Allowed unused vars must match /^_/u. (@typescript-eslint/no-unused-vars)
[notice] 2-2: src/features/projectsV2/ProjectsMap/MapFeatureExplorer/microComponents/SingleLayerOption.tsx#L2
'ReactNode' is defined but never used. Allowed unused vars must match /^_/u. (@typescript-eslint/no-unused-vars)
[notice] 5-5: src/features/projectsV2/ProjectsMap/MapFeatureExplorer/microComponents/SingleLayerOption.tsx#L5
'useState' is defined but never used. Allowed unused vars must match /^_/u. (@typescript-eslint/no-unused-vars)
[notice] 2-2: src/features/projectsV2/ProjectsMap/MapFeatureExplorer/microComponents/SingleLayerOption.tsx#L2
'ChangeEvent' is defined but never used. Allowed unused vars must match /^_/u. (@typescript-eslint/no-unused-vars)
[notice] 11-11: src/features/projectsV2/ProjectsMap/MapFeatureExplorer/microComponents/SingleLayerOption.tsx#L11
'MapOptions' is defined but never used. Allowed unused vars must match /^_/u. (@typescript-eslint/no-unused-vars)
src/features/projectsV2/ProjectsMap/MapFeatureExplorer/MapSettings.tsx
[notice] 11-11: src/features/projectsV2/ProjectsMap/MapFeatureExplorer/MapSettings.tsx#L11
'themeProperties' is defined but never used. Allowed unused vars must match /^_/u. (@typescript-eslint/no-unused-vars)
[notice] 24-24: src/features/projectsV2/ProjectsMap/MapFeatureExplorer/MapSettings.tsx#L24
'mapOptions' is defined but never used. Allowed unused args must match /^_/u. (@typescript-eslint/no-unused-vars)
[notice] 25-25: src/features/projectsV2/ProjectsMap/MapFeatureExplorer/MapSettings.tsx#L25
'updateMapOption' is defined but never used. Allowed unused args must match /^_/u. (@typescript-eslint/no-unused-vars)
src/features/projectsV2/ProjectsMap/MapFeatureExplorer/microComponents/MapSettingsSection.tsx
[notice] 1-1: src/features/projectsV2/ProjectsMap/MapFeatureExplorer/microComponents/MapSettingsSection.tsx#L1
'ChangeEvent' is defined but never used. Allowed unused vars must match /^_/u. (@typescript-eslint/no-unused-vars)
[notice] 4-4: src/features/projectsV2/ProjectsMap/MapFeatureExplorer/microComponents/MapSettingsSection.tsx#L4
'MapSettingsConfig' is defined but never used. Allowed unused vars must match /^_/u. (@typescript-eslint/no-unused-vars)
[notice] 8-8: src/features/projectsV2/ProjectsMap/MapFeatureExplorer/microComponents/MapSettingsSection.tsx#L8
'StyledSwitch' is defined but never used. Allowed unused vars must match /^_/u. (@typescript-eslint/no-unused-vars)
[notice] 6-6: src/features/projectsV2/ProjectsMap/MapFeatureExplorer/microComponents/MapSettingsSection.tsx#L6
'MapOptions' is defined but never used. Allowed unused vars must match /^_/u. (@typescript-eslint/no-unused-vars)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (javascript)
🔇 Additional comments (5)
src/utils/mapsV2/mapSettings.config.ts (1)
126-127: Verify the validity of future-dated source.The canopy height data source references "Tolan et al. 2024" and points to an April 2024 URL. Since this appears to be a future date, please verify if this is correct or needs updating.
src/features/projectsV2/ProjectsMap/MapFeatureExplorer/CustomSwitch.tsx (1)
38-38: LGTM! Well-implemented theme fallback.The addition of the theme color fallback (
themeProperties.primaryDarkColor) improves component reliability by ensuring a consistent appearance whencustomColoris not provided.Also applies to: 41-41
src/features/projectsV2/ProjectsMap/MapFeatureExplorer/MapFeatureExplorer.module.scss (1)
48-82: Well-structured styling for sections and layer options!The implementation provides consistent spacing, proper alignment, and handles borders correctly. Good use of BEM-like naming convention.
src/features/projectsV2/ProjectsMap/MapFeatureExplorer/MapSettings.tsx (2)
33-43: Well-structured configuration-driven sections!Good use of configuration object to drive the rendering of different map setting sections.
69-75: Clean mobile layout implementation!Good use of conditional rendering for mobile view with proper prop handling.
| {/* {hasInfoPopover && ( | ||
| <Popover | ||
| open={Boolean(anchorEl)} | ||
| onClose={() => setAnchorEl(null)} | ||
| anchorEl={anchorEl} | ||
| anchorOrigin={{ | ||
| vertical: 'top', | ||
| horizontal: 'left', | ||
| }} | ||
| transformOrigin={{ | ||
| vertical: 'top', | ||
| horizontal: 'center', | ||
| }} | ||
| disableRestoreFocus | ||
| sx={{ | ||
| pointerEvents: 'auto', // Enable pointer events | ||
| '& .MuiPaper-root': { | ||
| borderRadius: '12px', | ||
| marginTop: '-8px', | ||
| pointerEvents: 'auto', // Ensure interaction inside Popover works | ||
| }, | ||
| }} | ||
| > | ||
| <LayerInfoPopupContent | ||
| anchorEl={anchorEl} | ||
| setAnchorEl={setAnchorEl} | ||
| additionalInfo={layerConfig.additionalInfo} | ||
| handleMouseLeave={handleMouseLeave} | ||
| /> | ||
| </Popover> | ||
| )} */} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Clean up or implement commented-out popover code.
There's a significant block of commented-out code related to popover functionality. Either implement this feature or remove the commented code as suggested in the PR comments.
Would you like me to help implement the popover functionality for displaying layer information?
| <p>{tExplore(`settingsLabels.${layerConfig.key}`)}</p> | ||
| </div> | ||
| <div className={styles.switchContainer}> | ||
| <StyledSwitch customColor={layerConfig.color} /> |
There was a problem hiding this comment.
Add onChange handler to StyledSwitch.
The switch component is missing an onChange handler, making it non-interactive. Consider adding the handler to manage the layer's visibility state.
- <StyledSwitch customColor={layerConfig.color} />
+ <StyledSwitch
+ customColor={layerConfig.color}
+ onChange={(event: React.ChangeEvent<HTMLInputElement>) => {
+ // TODO: Implement layer visibility toggle
+ console.log(`Layer ${layerConfig.key} toggled:`, event.target.checked);
+ }}
+ />Would you like me to help implement the layer visibility toggle functionality?
Committable suggestion skipped: line range outside the PR's diff.
| /* interface Props { | ||
| category?: string; | ||
| exploreConfig: { | ||
| label: string; | ||
| color: string | undefined; | ||
| showDivider: boolean; | ||
| additionalInfo?: AdditionalInfo; | ||
| shouldRender: boolean; | ||
| }[]; | ||
| mapOptions?: MapOptions; | ||
| updateMapOption?: (option: keyof MapOptions, value: boolean) => void; | ||
| } */ |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove commented-out code.
There are large blocks of commented-out code that should be removed to improve code maintainability, as suggested in the PR comments.
Also applies to: 75-100
| .layerInfoPopupContainer { | ||
| display: flex; | ||
| flex-direction: column; | ||
| gap: 8px; | ||
| max-width: 189px; | ||
| background: #fff; | ||
| padding: 12px; | ||
| font-size: 10px; | ||
|
|
||
| .label { | ||
| font-weight: 600; | ||
| color: rgba(130, 130, 130, 1); | ||
| text-transform: uppercase; | ||
| } | ||
| .source { | ||
| color: $primaryDarkColor; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
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
.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;
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| .layerInfoPopupContainer { | |
| display: flex; | |
| flex-direction: column; | |
| gap: 8px; | |
| max-width: 189px; | |
| background: #fff; | |
| padding: 12px; | |
| font-size: 10px; | |
| .label { | |
| font-weight: 600; | |
| color: rgba(130, 130, 130, 1); | |
| text-transform: uppercase; | |
| } | |
| .source { | |
| color: $primaryDarkColor; | |
| } | |
| .layerInfoPopupContainer { | |
| display: flex; | |
| flex-direction: column; | |
| gap: 8px; | |
| max-width: min(189px, 90vw); | |
| background: #fff; | |
| padding: 12px; | |
| font-size: 12px; | |
| .label { | |
| font-weight: 600; | |
| color: rgba(130, 130, 130, 1); | |
| text-transform: uppercase; | |
| } | |
| .source { | |
| color: $primaryDarkColor; | |
| word-break: break-word; | |
| } | |
| } |
| const MapSettings: FC<MapSettingsProps> = ({ | ||
| mapOptions, | ||
| updateMapOption, | ||
| isMobile, | ||
| setIsOpen, | ||
| }) => { |
There was a problem hiding this comment.
Address unused props flagged by static analysis.
The mapOptions and updateMapOption props are defined but never used in the component.
Either remove these unused props or implement their usage in the component.
🧰 Tools
🪛 GitHub Check: CodeFactor
[notice] 24-24: src/features/projectsV2/ProjectsMap/MapFeatureExplorer/MapSettings.tsx#L24
'mapOptions' is defined but never used. Allowed unused args must match /^_/u. (@typescript-eslint/no-unused-vars)
[notice] 25-25: src/features/projectsV2/ProjectsMap/MapFeatureExplorer/MapSettings.tsx#L25
'updateMapOption' is defined but never used. Allowed unused args must match /^_/u. (@typescript-eslint/no-unused-vars)
| {/* <MapSettingsSection | ||
| exploreConfig={projectConfig} | ||
| updateMapOption={updateMapOption} | ||
| mapOptions={mapOptions} | ||
| /> | ||
| <MapSettingsSection | ||
| category={tMaps('layers.forests')} | ||
| exploreConfig={forestConfig} | ||
| /> | ||
| <MapSettingsSection | ||
| category={tMaps('layers.forests')} | ||
| exploreConfig={forestConfig} | ||
| /> | ||
| <MapSettingsSection | ||
| category={tMaps('layers.forests')} | ||
| exploreConfig={forestConfig} | ||
| /> | ||
| <MapSettingsSection | ||
| category={tMaps('layers.forests')} | ||
| exploreConfig={forestConfig} | ||
| /> */} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove commented MapSettingsSection implementations.
Clean up the commented code to improve maintainability.
- {/* <MapSettingsSection
- exploreConfig={projectConfig}
- updateMapOption={updateMapOption}
- mapOptions={mapOptions}
- />
- <MapSettingsSection
- category={tMaps('layers.forests')}
- exploreConfig={forestConfig}
- />
- ... more commented sections ... */}| // // import { MapLayerToggle } from '.'; | ||
| // import InfoIcon from '../../../../../public/assets/images/icons/projectV2/InfoIcon'; | ||
| import { StyledSwitch } from './CustomSwitch'; | ||
| // import { StyledSwitch } from './CustomSwitch'; | ||
| // import { YearRangeSlider } from '.'; | ||
| import { useTranslations } from 'next-intl'; | ||
| // import themeProperties from '../../../../theme/themeProperties'; | ||
| import themeProperties from '../../../../theme/themeProperties'; | ||
| import MapSettingsSection from './microComponents/MapSettingsSection'; | ||
| import MobileMapSettingsLayout from './MobileMapSettingsLayout'; | ||
| import { mapSettingsConfig } from '../../../../utils/mapsV2/mapSettings.config'; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Clean up commented imports and unused code.
Remove commented imports to improve code maintainability.
-// // import { MapLayerToggle } from '.';
-// import InfoIcon from '../../../../../public/assets/images/icons/projectV2/InfoIcon';
-// import { StyledSwitch } from './CustomSwitch';
-// import { YearRangeSlider } from '.';📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // // import { MapLayerToggle } from '.'; | |
| // import InfoIcon from '../../../../../public/assets/images/icons/projectV2/InfoIcon'; | |
| import { StyledSwitch } from './CustomSwitch'; | |
| // import { StyledSwitch } from './CustomSwitch'; | |
| // import { YearRangeSlider } from '.'; | |
| import { useTranslations } from 'next-intl'; | |
| // import themeProperties from '../../../../theme/themeProperties'; | |
| import themeProperties from '../../../../theme/themeProperties'; | |
| import MapSettingsSection from './microComponents/MapSettingsSection'; | |
| import MobileMapSettingsLayout from './MobileMapSettingsLayout'; | |
| import { mapSettingsConfig } from '../../../../utils/mapsV2/mapSettings.config'; | |
| import { useTranslations } from 'next-intl'; | |
| import themeProperties from '../../../../theme/themeProperties'; | |
| import MapSettingsSection from './microComponents/MapSettingsSection'; | |
| import MobileMapSettingsLayout from './MobileMapSettingsLayout'; | |
| import { mapSettingsConfig } from '../../../../utils/mapsV2/mapSettings.config'; |
🧰 Tools
🪛 GitHub Check: CodeFactor
[notice] 11-11: src/features/projectsV2/ProjectsMap/MapFeatureExplorer/MapSettings.tsx#L11
'themeProperties' is defined but never used. Allowed unused vars must match /^_/u. (@typescript-eslint/no-unused-vars)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (12)
src/features/projectsV2/ProjectsMap/MapFeatureExplorer/MapSettings.tsx (2)
6-10: Clean up commented imports.Remove commented-out imports to improve code maintainability.
-// // import { MapLayerToggle } from '.'; -// import InfoIcon from '../../../../../public/assets/images/icons/projectV2/InfoIcon'; -// import { StyledSwitch } from './CustomSwitch'; -// import { YearRangeSlider } from '.';
58-64: Remove commented MapLayerToggle implementation.Clean up the commented code to improve maintainability.
- {/* <MapLayerToggle - infoIcon={<InfoIcon width={'10px'} />} - label={tAllProjects('deforestation')} - switchComponent={ - <StyledSwitch customColor={`${deforestrationToggleColorNew}`} /> - } - /> */}src/features/projectsV2/ProjectsMap/MapFeatureExplorer/microComponents/LayerInfoPopupContent.tsx (1)
28-28: Consider using clearTimeout for cleanup.The setTimeout call could lead to race conditions if the component unmounts before the timeout completes.
+ const [timeoutId, setTimeoutId] = useState<NodeJS.Timeout>(); + + useEffect(() => { + return () => { + if (timeoutId) { + clearTimeout(timeoutId); + } + }; + }, [timeoutId]); - onMouseLeave={() => setTimeout(handleMouseLeave, 500)} + onMouseLeave={() => { + const id = setTimeout(handleMouseLeave, 500); + setTimeoutId(id); + }}src/features/projectsV2/ProjectsMap/MapFeatureExplorer/microComponents/SingleLayerOption.tsx (2)
12-16: Remove commented interface props.Clean up the commented interface props to improve code maintainability.
- /* label: string; - switchComponent: ReactNode; - showDivider: boolean; - additionalInfo?: AdditionalInfo; */
66-72: Improve popover accessibility.The popover's pointer events and styling could be improved for better accessibility.
sx={{ - pointerEvents: 'auto', // Enable pointer events + pointerEvents: 'none', // Prevent pointer events on the popover container '& .MuiPaper-root': { borderRadius: '12px', marginTop: '-8px', - pointerEvents: 'auto', // Ensure interaction inside Popover works + pointerEvents: 'auto', // Enable pointer events only on the content + outline: 'none', // Remove focus ring }, }}src/features/projectsV2/ProjectsMap/MapFeatureExplorer/microComponents/MapSettingsSection.tsx (2)
8-19: Remove commented interface.Clean up the commented interface to improve code maintainability.
-/* interface Props { - category?: string; - exploreConfig: { - label: string; - color: string | undefined; - showDivider: boolean; - additionalInfo?: AdditionalInfo; - shouldRender: boolean; - }[]; - mapOptions?: MapOptions; - updateMapOption?: (option: keyof MapOptions, value: boolean) => void; -} */
41-41: Consider using null instead of empty fragment.Using an empty fragment as a return value could be replaced with null for better readability.
- if (!layerConfig.isAvailable) return <></>; + if (!layerConfig.isAvailable) return null;src/utils/mapsV2/mapSettings.config.ts (5)
31-36: Add JSDoc comments and improve type safety.Consider these enhancements to the
LayerConfiginterface:
- Add JSDoc comments to explain when optional properties are expected
- Use a more specific type for the color property (e.g., CSS color string)
export interface LayerConfig { key: MapSettings; isAvailable: boolean; + /** Additional metadata about the layer. Required for all layers except 'projects' */ additionalInfo?: AdditionalInfo; + /** CSS color string for layer visualization. Required for layers that support custom coloring */ - color?: string; + color?: `#${string}`; }
20-29: Consider URL validation for source links.The
source.urlproperty should be validated to ensure it's a valid URL. Consider using a URL validation type or adding runtime validation.export interface AdditionalInfo { resolution: string; dataYears: string; description: string; underlyingData: string; source: { text: string; - url: string; + url: `https://${string}` | `http://${string}`; }; }
140-140: Fix typo in soil descriptions.There's a typo in the word "horrizon" (should be "horizon") across multiple soil layer descriptions.
- 'cg/kg; 0-30 cm horrizon (A weighted average for all depths)', + 'cg/kg; 0-30 cm horizon (A weighted average for all depths)',Also applies to: 156-156, 171-171, 187-187
58-58: Standardize date formats and verify data years.The
dataYearsfield uses inconsistent formats across different layers:
- Single year: '2023'
- Range with en dash: '2018–2020'
- Range with hyphen: '2013-2018'
Consider standardizing the format across all layers.
Also applies to: 74-74, 89-89, 106-106, 121-121, 138-138, 154-154, 169-169, 185-185, 203-203, 219-219, 235-235, 250-250, 267-267, 281-281
140-140: Standardize unit formatting.The units in descriptions are formatted inconsistently:
- Some use semicolons: "cg/kg; 0-30 cm"
- Some have duplicate units: "cg/cm3; cg/kg"
Consider standardizing the unit formatting across all descriptions.
Also applies to: 171-171, 187-187
📜 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 (6)
src/features/projectsV2/ProjectsMap/MapFeatureExplorer/MapFeatureExplorer.module.scss(2 hunks)src/features/projectsV2/ProjectsMap/MapFeatureExplorer/MapSettings.tsx(1 hunks)src/features/projectsV2/ProjectsMap/MapFeatureExplorer/microComponents/LayerInfoPopupContent.tsx(1 hunks)src/features/projectsV2/ProjectsMap/MapFeatureExplorer/microComponents/MapSettingsSection.tsx(1 hunks)src/features/projectsV2/ProjectsMap/MapFeatureExplorer/microComponents/SingleLayerOption.tsx(1 hunks)src/utils/mapsV2/mapSettings.config.ts(1 hunks)
🧰 Additional context used
🪛 GitHub Check: CodeFactor
src/features/projectsV2/ProjectsMap/MapFeatureExplorer/microComponents/MapSettingsSection.tsx
[notice] 6-6: src/features/projectsV2/ProjectsMap/MapFeatureExplorer/microComponents/MapSettingsSection.tsx#L6
'MapOptions' is defined but never used. Allowed unused vars must match /^_/u. (@typescript-eslint/no-unused-vars)
src/features/projectsV2/ProjectsMap/MapFeatureExplorer/MapSettings.tsx
[notice] 24-24: src/features/projectsV2/ProjectsMap/MapFeatureExplorer/MapSettings.tsx#L24
'mapOptions' is defined but never used. Allowed unused args must match /^_/u. (@typescript-eslint/no-unused-vars)
[notice] 25-25: src/features/projectsV2/ProjectsMap/MapFeatureExplorer/MapSettings.tsx#L25
'updateMapOption' is defined but never used. Allowed unused args must match /^_/u. (@typescript-eslint/no-unused-vars)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (javascript)
🔇 Additional comments (7)
src/features/projectsV2/ProjectsMap/MapFeatureExplorer/MapSettings.tsx (1)
22-27:⚠️ Potential issueRemove unused props.
The
mapOptionsandupdateMapOptionprops are defined but never used in the component.-const MapSettings: FC<MapSettingsProps> = ({ - mapOptions, - updateMapOption, - isMobile, - setIsOpen, -}) => { +const MapSettings: FC<MapSettingsProps> = ({ + isMobile, + setIsOpen, +}) => {Also update the type definition:
type MapSettingsProps = { - mapOptions: MapOptions; - updateMapOption: (option: keyof MapOptions, value: boolean) => void; isMobile?: boolean; setIsOpen?: SetState<boolean>; };Likely invalid or redundant comment.
🧰 Tools
🪛 GitHub Check: CodeFactor
[notice] 24-24: src/features/projectsV2/ProjectsMap/MapFeatureExplorer/MapSettings.tsx#L24
'mapOptions' is defined but never used. Allowed unused args must match /^_/u. (@typescript-eslint/no-unused-vars)
[notice] 25-25: src/features/projectsV2/ProjectsMap/MapFeatureExplorer/MapSettings.tsx#L25
'updateMapOption' is defined but never used. Allowed unused args must match /^_/u. (@typescript-eslint/no-unused-vars)src/features/projectsV2/ProjectsMap/MapFeatureExplorer/microComponents/LayerInfoPopupContent.tsx (1)
64-70:⚠️ Potential issueValidate source URL before rendering.
When rendering
hreffromadditionalInfo.source.url, consider validating the URL to prevent potential security risks.+ const isValidUrl = (url: string) => { + try { + new URL(url); + return url.startsWith('http://') || url.startsWith('https://'); + } catch { + return false; + } + }; <a - href={additionalInfo.source.url} + href={isValidUrl(additionalInfo.source.url) ? additionalInfo.source.url : '#'} target="_blank" rel="noopener noreferrer" className={styles.source} >Likely invalid or redundant comment.
src/features/projectsV2/ProjectsMap/MapFeatureExplorer/microComponents/SingleLayerOption.tsx (1)
48-48:⚠️ Potential issueAdd onChange handler to StyledSwitch.
The switch component is missing an onChange handler, making it non-interactive.
- <StyledSwitch customColor={layerConfig.color} /> + <StyledSwitch + customColor={layerConfig.color} + onChange={(event: React.ChangeEvent<HTMLInputElement>) => { + // Handle layer visibility + console.log(`Layer ${layerConfig.key} toggled:`, event.target.checked); + }} + />Would you like me to help implement the layer visibility toggle functionality?
Likely invalid or redundant comment.
src/features/projectsV2/ProjectsMap/MapFeatureExplorer/MapFeatureExplorer.module.scss (1)
89-105: 🛠️ Refactor suggestionAddress 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; } }Likely invalid or redundant comment.
src/utils/mapsV2/mapSettings.config.ts (3)
1-19: Well-structured type definition!The
MapSettingstype effectively captures all possible map layer categories with clear, descriptive names.
126-128: Verify the publication date.The source references a future date (2024) for the Tolan et al. publication. Please verify if this is correct.
47-293: Configuration structure is robust and comprehensive!The configuration object provides detailed metadata for each layer with consistent structure and comprehensive documentation. The minor issues noted above don't impact functionality but would improve maintainability.
This PR handles the new explore drop down UI (mobile, desktop).