Issue 154: Migration of custom UI Components to Material UI (MUI) Components + Light/Dark Theme #181
Conversation
📝 WalkthroughWalkthroughIntroduces Material UI (MUI) to the frontend, adds a ColorModeContext-based light/dark theming system with persistence, replaces custom UI primitives with MUI components, refactors map and layout components to use MUI primitives, and reorganizes CSS into theme-aware variables and component styles. Changes
Sequence DiagramsequenceDiagram
participant User
participant Main as Main.tsx
participant Context as ColorModeContext
participant App
participant Header
User->>Main: Load app
Main->>Main: Init mode (localStorage/default)
Main->>Main: Build MUI theme
Main->>Context: Provide {mode, toggleMode}
Main->>App: Render App with ThemeProvider
App->>App: Load collections (FixtureReader)
App->>MapContainer: Render layers
App->>LayerControls: Render visibility map
User->>Header: Click dark mode toggle
Header->>Context: Call toggleMode()
Context->>Main: Update mode state
Main->>Main: Persist mode to localStorage
Main->>App: Re-render with new theme
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@src/components/Map/Layer.tsx`:
- Around line 20-23: The key generation for the GeoJSON child currently uses
`${collection.name}-${feature.id || idx}` which treats falsy but valid IDs (like
0 or empty string) as absent; update the key construction in the map callback
that renders <GeoJSON ...> to use nullish coalescing so only null/undefined fall
back to the index (i.e., use feature.id ?? idx) while keeping the
`${collection.name}-...` prefix and the same map/GeoJSON rendering logic.
In `@src/components/Map/LayerControls.tsx`:
- Around line 23-39: The Box container uses
aria-labelledby="layer-controls-title" but the header lacks that id; update the
header element (the h3 with className "gen-header controls-header") to include
id="layer-controls-title" so the ARIA reference is valid (alternatively remove
the aria-labelledby attribute on the Box if you prefer no label reference).
In `@src/styles/components.css`:
- Around line 148-151: The CSS rule .light-dark-controller:hover uses an
undefined variable --bg-hover-controller; update that reference to the existing
palette variable --bg-header-controller-hover (replace --bg-hover-controller
with --bg-header-controller-hover in the .light-dark-controller:hover selector)
so the hover background uses the correct defined CSS custom property.
🧹 Nitpick comments (10)
src/styles/palette-layouts.css (2)
14-22: Redundant variable declarations in dark theme.Lines 16-17 redeclare
--color-primaryand--color-secondarywith the same values as the light theme. These can be removed since they're already defined in:rootand don't change between themes.♻️ Suggested simplification
/* MUI Dark Palette */ [data-theme='dark'] { --body-bg-default: `#16213e`; - --color-primary: `#667eea`; - --color-secondary: `#764ba2`; --color-bg-default: `#1a1a2e`; --color-bg-paper: rgba(0, 0, 0, 0.2); --color-text-primary: `#e2e8f0`; --color-text-secondary: `#94a3b8`; }
40-43: Inconsistent variable naming convention.
--zIndexuses camelCase while all other variables use kebab-case (e.g.,--width-controls). Consider renaming to--z-indexfor consistency.♻️ Suggested fix
/* Component System */ :root { --width-controls: 240px; - --zIndex: 1000; + --z-index: 1000; }src/styles/components.css (2)
61-71: Duplicate utility classes.
.flex-row-center(lines 61-65) and.flex-row-align-center(lines 67-71) have identical definitions. Consider removing one to avoid confusion.♻️ Suggested consolidation
.flex-row-center { display: flex; flex-direction: row; align-items: center; } -.flex-row-align-center { - display: flex; - flex-direction: row; - align-items: center; -}
260-291: Consider removing or tracking commented-out code.Lines 260-529 contain a large block of commented-out CSS. Per the PR summary, this was intentionally kept for reference during refactoring. Consider creating an issue to track the cleanup of this legacy code rather than keeping it in the codebase long-term.
Would you like me to open an issue to track the cleanup of this legacy styling code?
src/styles/reset.css (1)
14-20: Redundant margin and padding declarations.Lines 15-16 declare
margin: 0andpadding: 0onbody, but these are already reset by the universal selector on lines 3-5.♻️ Suggested cleanup
body { - margin: 0; - padding: 0; font-size: 18px; line-height: 1.5; font-family: -apple-system, BlinkMacSystemFont, 'Segoe UI', 'Roboto', 'Inter', sans-serif; }src/main.tsx (3)
17-20: MoveColorModeContextto a separate file for fast refresh compatibility.Exporting
ColorModeContextfrom the same file as theMaincomponent breaks React fast refresh during development. Extract the context to a dedicated file (e.g.,src/contexts/ColorModeContext.tsx).♻️ Suggested refactor
Create
src/contexts/ColorModeContext.tsx:import { createContext } from 'react'; interface ColorModeContextType { toggleMode: () => void; mode: 'light' | 'dark'; } export const ColorModeContext = createContext<ColorModeContextType>({ toggleMode: () => {}, mode: 'light', });Then import it in
main.tsx:-export const ColorModeContext = createContext<ColorModeContextType>({ - toggleMode: () => {}, - mode: 'light', -}); +import { ColorModeContext } from '@/contexts/ColorModeContext';
39-44: Add error handling for localStorage access.
localStorage.getItemcan throw in restricted environments (e.g., private browsing mode in some browsers, or when storage is disabled). Wrap in try-catch to prevent runtime errors.♻️ Suggested fix
const [mode, setMode] = useState<'light' | 'dark'>(() => { - const storedPalette = localStorage.getItem('themePalette'); - const activePalette = storedPalette === 'dark' ? 'dark' : 'light'; + let activePalette: 'light' | 'dark' = 'light'; + try { + const storedPalette = localStorage.getItem('themePalette'); + activePalette = storedPalette === 'dark' ? 'dark' : 'light'; + } catch { + // localStorage unavailable, use default + } document.documentElement.setAttribute('data-theme', activePalette); return activePalette; });
51-58: Add error handling for localStorage in toggleMode.Similarly,
localStorage.setItemshould be wrapped in try-catch to handle restricted environments gracefully.♻️ Suggested fix
const toggleMode = () => { setMode((currentPalette) => { const nextPalette = currentPalette === 'light' ? 'dark' : 'light'; document.documentElement.setAttribute('data-theme', nextPalette); - localStorage.setItem('themePalette', nextPalette); + try { + localStorage.setItem('themePalette', nextPalette); + } catch { + // localStorage unavailable, theme still works in-memory + } return nextPalette; }); };src/components/Map/MapContainer.tsx (1)
53-55: Uselayer.nameas key instead of array index.Per the
FeatureCollectiontype insrc/types/geometry.ts, each layer has anameproperty. Using this as the key provides stable identity if layers are reordered or filtered.♻️ Suggested fix
- {layers.map((layer, idx) => ( - <Layer key={idx} collection={layer} /> + {layers.map((layer) => ( + <Layer key={layer.name} collection={layer} /> ))}src/App.tsx (1)
45-74: Add basic error handling (and guard async updates) when loading collections.If
FixtureReader.collections()throws, the promise rejection is unhandled and the UI silently stays empty. A small try/catch plus an unmount guard makes the behavior safer.🔧 Suggested change
useEffect(() => { - + let isMounted = true; async function loadCollections() { - - /* Fetch feature collections and save layers to state */ - const collections: FeatureCollection[] = await FixtureReader.collections(); - const allLayers = [... collections]; - setLayers(allLayers); + try { + /* Fetch feature collections and save layers to state */ + const collections: FeatureCollection[] = await FixtureReader.collections(); + if (!isMounted) return; + const allLayers = [...collections]; + setLayers(allLayers); - /* Extract layer names */ - const layerNames = allLayers.map((layer) => layer.name); + /* Extract layer names */ + const layerNames = allLayers.map((layer) => layer.name); - /* Build initial visibility map */ - const initialVisibility: LayerVisibilityMap = layerNames.reduce( - (map, name) => { - map[name] = true; - return map; - }, - {} as LayerVisibilityMap - ); + /* Build initial visibility map */ + const initialVisibility: LayerVisibilityMap = layerNames.reduce( + (map, name) => { + map[name] = true; + return map; + }, + {} as LayerVisibilityMap + ); - /* Save visibility map to state */ - setLayerVisibility(initialVisibility); + /* Save visibility map to state */ + setLayerVisibility(initialVisibility); + } catch (err) { + console.error('Failed to load map collections', err); + } } loadCollections(); + return () => { + isMounted = false; + }; }, [] // dependency array )
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/components/Map/LayerControls.tsx`:
- Around line 41-42: Remove the explicit role="group" on the <ul> in
LayerControls so native list semantics are preserved; keep the aria-label (e.g.,
"Map Data Layers") for an accessible name. Locate the UL rendered in the
LayerControls component that maps over visibilityMap
(Object.entries(visibilityMap).map(...)) and remove the role attribute while
leaving aria-label intact to ensure screen readers announce it as a list.
🧹 Nitpick comments (1)
src/components/Map/LayerControls.tsx (1)
78-107: Consider removing the legacy commented block once migration is stable.It adds noise and can drift out of sync; capturing it in docs or an issue might be cleaner.
| <ul role="group" aria-label="Map Data Layers"> | ||
| {Object.entries(visibilityMap).map(([layerName, isVisible]) => ( |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
WAI-ARIA role="group" on ul element list semantics assistive technology
💡 Result:
Short answer: you generally should not add role="group" to a semantic
- ; use native
- semantics or nested lists instead. Applying role="group" is allowed in a list context (its children must be listitem), but it changes how AT treats the container (it’s a generic grouping, not a list landmark) and can alter what screen readers announce. Use role="list"/"listitem" only when you can’t use proper HTML. [1][2][3]
Key points and guidance
- The group role models a generic grouping (like a fieldset) and when used inside a list its children must be listitem elements. It is intended for logical collections that are not page landmarks. Assistive tech receives a “group” notification rather than a list-region announcement. [1][2]
- Best practice: prefer semantic HTML (
- /
- ) because browsers/AT expose that reliably. Only add ARIA roles (role="list"/role="listitem"/role="group") when you cannot change the underlying markup. [2][3]
- Risk: forcing role changes can suppress or change implicit semantics (e.g., using role="presentation" on a list hides list semantics), and different AT/browser combos may announce group vs list differently. Test with major screen readers (NVDA, VoiceOver, JAWS) if you must change roles. [3][4]
- /
Sources
- MDN: role="group" and guidance. [1]
- MDN: list/listitem roles and best practices (use native
- /
- ). [2]
- W3C HTML+ARIA guidance (don’t override native semantics; role=list rarely needed). [3]
- /
Preserve native list semantics by removing role="group".
Using role="group" on a <ul> suppresses list semantics for assistive technology; screen readers will announce it as a generic grouping rather than a list. Remove the role and keep the label via aria-label to maintain proper list announcements.
Suggested change
- <ul role="group" aria-label="Map Data Layers">
+ <ul aria-label="Map Data Layers">📝 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.
| <ul role="group" aria-label="Map Data Layers"> | |
| {Object.entries(visibilityMap).map(([layerName, isVisible]) => ( | |
| <ul aria-label="Map Data Layers"> | |
| {Object.entries(visibilityMap).map(([layerName, isVisible]) => ( |
🤖 Prompt for AI Agents
In `@src/components/Map/LayerControls.tsx` around lines 41 - 42, Remove the
explicit role="group" on the <ul> in LayerControls so native list semantics are
preserved; keep the aria-label (e.g., "Map Data Layers") for an accessible name.
Locate the UL rendered in the LayerControls component that maps over
visibilityMap (Object.entries(visibilityMap).map(...)) and remove the role
attribute while leaving aria-label intact to ensure screen readers announce it
as a list.
1 participant
Description
Refactored CSS into three files:
reset.cssfor default settings;palette-layout.cssfor color, spacing, and global variables; andcomponents.cssfor component level styling. CSS for layout and styling now found in TSX and CSS files, respectively. App allows users to toggle between light/dark mode with Material UI across Chrome, Firefox, and Edge browsers. Code that is no longer rendered has been commented out and placed at the bottom of relevant files. Suggest a new CSS file be created for responsive layouts. Updated overall documentation where necessary for clarity.Type of Change
How Has This Been Tested?
Ran app locally. No regressions across major browsers.
Checklist:
Related Issues
The Bi-monthly maintainer meeting issue document (#156) mentions several issues related to this one.
Summary by CodeRabbit
New Features
Style
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.