Issue 154: Migration of custom UI Components to Material UI (MUI) Components + Light/Dark Theme #171
Conversation
|
When I started this PR the App.tsx file commented out the MapLegend.tsx file as follows:
Therefore, I did not implement MUI support to the MapLegend.tsx file since it is not on the app when it runs. |
oscar-romero-jorge
left a comment
oscar-romero-jorge
left a comment
There was a problem hiding this comment.
nice!
Tested locally and functionality looks good.
- Dark/light mode toggle working smoothly
- MUI components rendering correctly
- No errors in console
MUI should make UI changes easier
|
Awesome! Learned a lot on this Pull Request. |
|
Just wanted to bring the merge conflicts to attention before merging. Thank you :) |
WalkthroughAdds MUI and Emotion dependencies; introduces a ColorModeContext and Main provider that builds and persists a MUI theme from CSS variables; refactors components to use MUI primitives and JSX typings; removes a custom Checkbox component; and consolidates styles into CSS-variable-driven global and map styles. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant Header as Header (UI)
participant Context as ColorModeContext
participant Storage as localStorage
participant Theme as ThemeProvider
participant App as App (components)
User->>Header: click theme toggle
Header->>Context: toggleColorMode()
Context->>Storage: persist mode
Context->>Theme: trigger theme rebuild (reads CSS vars)
Theme->>App: provide updated theme
Note over App,Header: Components re-render with new theme and [data-theme] applied
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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: 6
🧹 Nitpick comments (2)
src/main.tsx (1)
85-106: Consider consolidating CSS variable usage for consistency.The
GlobalStylesblock mixes MUI theme values (theme.palette.mode,theme.palette.text.primary) with direct CSS variable references (var(--body-bg-default-end),var(--box-shadow-light)). Line 91 references--body-bg-default-endwhich was not extracted into the theme palette. While this works, it creates split responsibility between the theme and raw CSS variables.For better consistency, consider either:
- Extract all CSS variables used in GlobalStyles into the theme palette
- Or use CSS variables consistently throughout without extracting to theme
This is a minor stylistic point that doesn't affect functionality but would improve maintainability.
src/components/Map/LayerControls.tsx (1)
76-122: Consider removing commented code now that migration is complete.The commented code block (47 lines) preserves the old implementation for reference. While this can be helpful during migration, the old code is preserved in Git history and can be retrieved if needed. Removing it would improve file readability.
This is a minor cleanup suggestion and can be deferred or declined based on team preference.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (10)
package.json(1 hunks)src/App.tsx(3 hunks)src/components/Layout/Header.tsx(1 hunks)src/components/Map/Layer.tsx(1 hunks)src/components/Map/LayerControls.tsx(1 hunks)src/components/Map/MapContainer.tsx(3 hunks)src/components/UI/CheckBox.tsx(0 hunks)src/main.tsx(1 hunks)src/styles/globals.css(1 hunks)src/styles/map.css(6 hunks)
💤 Files with no reviewable changes (1)
- src/components/UI/CheckBox.tsx
🧰 Additional context used
🧬 Code graph analysis (4)
src/components/Map/Layer.tsx (1)
src/types/geometry.ts (1)
FeatureCollection(4-12)
src/components/Map/LayerControls.tsx (1)
src/types/map.ts (1)
LayerVisibilityMap(5-7)
src/App.tsx (3)
src/types/map.ts (1)
LayerVisibilityMap(5-7)src/components/Map/MapContainer.tsx (1)
MapContainer(28-60)src/components/Map/LayerControls.tsx (1)
LayerControls(17-74)
src/components/Layout/Header.tsx (1)
src/main.tsx (1)
ColorModeContext(12-14)
🪛 Biome (2.1.2)
src/styles/map.css
[error] 144-144: CSS variables '--app-blur' is used without the 'var()' function
CSS variables should be used with the 'var()' function to ensure proper fallback behavior and browser compatibility.
(lint/correctness/noMissingVarFunction)
🪛 ESLint
src/main.tsx
[error] 22-22: Fast refresh only works when a file only exports components. Move your component(s) to a separate file.
(react-refresh/only-export-components)
🔇 Additional comments (3)
src/components/Layout/Header.tsx (2)
28-54: LGTM! Good semantic structure and accessibility.The header structure uses MUI Box components with appropriate semantic HTML (
component="header",role="banner") and accessibility attributes (aria-label,aria-hidden). The logo link is properly labeled for screen readers.
75-102: LGTM! Well-implemented accessible toggle control.The theme toggle uses MUI IconButton with proper accessibility semantics (
role="switch",aria-checked,aria-label) and conditional icon rendering. ThedisableRippleis appropriate for a custom-styled toggle.Note: The
isDarkModestate issue is addressed in a previous comment.src/components/Map/LayerControls.tsx (1)
22-39: LGTM! Well-structured MUI migration with excellent accessibility.The component successfully migrates to MUI while maintaining strong accessibility:
- Semantic structure with
component="section",role="region", andaria-labelledby- MUI Checkbox with proper
slotPropsfor ARIA attributes (lines 60-65)- Clean separation of layout (sx props) and logic
The implementation follows React and accessibility best practices.
Also applies to: 55-69
src/components/Layout/Header.tsx
Outdated
| const { toggleColorMode } = useContext(ColorModeContext); // get toggle function from context | ||
| const [isDarkMode, setIsDarkMode] = useState(false); | ||
|
|
||
| // Load dark mode preference from localStorage on mount | ||
| useEffect(() => { | ||
| const savedTheme = localStorage.getItem('darkMode'); | ||
| if (savedTheme === 'true') { | ||
| setIsDarkMode(true); | ||
| document.documentElement.classList.add('dark'); | ||
| } | ||
| setIsDarkMode(savedTheme === 'dark'); | ||
| }, []); |
There was a problem hiding this comment.
Remove duplicated state management to prevent sync issues.
The component maintains its own isDarkMode state and reads from localStorage independently (lines 16-19), while main.tsx is already the source of truth for theme mode. This duplication can cause the UI to show the wrong icon/label if:
- Another component toggles the theme
- The mode is changed externally
- The component remounts
The ColorModeContext should expose the current mode, not just the toggle function.
Update src/main.tsx to expose the current mode in the context:
interface ColorModeContextType {
toggleColorMode: () => void;
+ mode: 'light' | 'dark';
}
export const ColorModeContext = createContext<ColorModeContextType>({
toggleColorMode: () => {},
+ mode: 'light',
});Then update the provider value:
- const colorMode = useMemo(() => ({ toggleColorMode }), []);
+ const colorMode = useMemo(() => ({ toggleColorMode, mode }), [mode]);Finally, update Header.tsx to read mode from context:
export const Header: React.FC = () => {
- const { toggleColorMode } = useContext(ColorModeContext);
- const [isDarkMode, setIsDarkMode] = useState(false);
-
- // Load dark mode preference from localStorage on mount
- useEffect(() => {
- const savedTheme = localStorage.getItem('darkMode');
- setIsDarkMode(savedTheme === 'dark');
- }, []);
+ const { toggleColorMode, mode } = useContext(ColorModeContext);
+ const isDarkMode = mode === 'dark';
// Toggle dark mode using context
const handleToggle = () => {
toggleColorMode();
- setIsDarkMode((prev) => !prev);
};Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/components/Layout/Header.tsx around lines 12 to 19, remove the duplicated
local isDarkMode state and the useEffect that reads localStorage; instead read
the current theme mode from ColorModeContext (so update the context consumer to
destructure both { toggleColorMode, mode } or similar) and use that mode to
determine icons/labels. Also update src/main.tsx to make the context value
include the current mode (e.g., provide { toggleColorMode, mode: currentMode })
when creating the provider so Header can rely on the context as the single
source of truth.
src/main.tsx
Outdated
| // 1️⃣ Define and export ColorModeContext | ||
| interface ColorModeContextType { | ||
| toggleColorMode: () => void; | ||
| } | ||
|
|
||
| export const ColorModeContext = createContext<ColorModeContextType>({ | ||
| toggleColorMode: () => {}, | ||
| }); |
There was a problem hiding this comment.
Address fast refresh limitation by moving context to a separate file.
The ESLint warning is valid: mixing component and context exports in the same file breaks React Fast Refresh during development. Extract ColorModeContextType and ColorModeContext to a dedicated file (e.g., src/contexts/ColorModeContext.tsx), then import them here.
Apply this refactor:
Create src/contexts/ColorModeContext.tsx:
import { createContext } from 'react';
export interface ColorModeContextType {
toggleColorMode: () => void;
}
export const ColorModeContext = createContext<ColorModeContextType>({
toggleColorMode: () => {},
});Then update src/main.tsx:
-import React, { useMemo, useState, useEffect, createContext } from 'react';
+import React, { useMemo, useState, useEffect } from 'react';
import ReactDOM from 'react-dom/client';
import App from './App.tsx';
import { ThemeProvider, createTheme, CssBaseline, GlobalStyles } from '@mui/material';
+import { ColorModeContext } from '@/contexts/ColorModeContext';
import '@/styles/globals.css';
-// 1️⃣ Define and export ColorModeContext
-interface ColorModeContextType {
- toggleColorMode: () => void;
-}
-
-export const ColorModeContext = createContext<ColorModeContextType>({
- toggleColorMode: () => {},
-});
-
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/contexts/ColorModeContext.tsx (1)
1-11: LGTM! Clean context implementation.The ColorModeContext is correctly typed and follows React best practices. The default no-op
toggleColorModeand'light'mode provide sensible fallbacks.Optional: Add defensive warning for better DX.
Consider adding a console warning in the default
toggleColorModeto help developers catch accidental usage outside the provider:export const ColorModeContext = createContext<ColorModeContextType>({ - toggleColorMode: () => {}, + toggleColorMode: () => { + console.warn('ColorModeContext: toggleColorMode called outside provider'); + }, mode: 'light', // 👈 Default value });This is purely optional and helps surface integration mistakes during development.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
src/main.tsx (1)
22-126: Consider extracting Main to a separate file for Fast Refresh support.The ESLint warning is accurate: mixing the
Maincomponent definition with the entry-point logic can break React Fast Refresh during development. While this works functionally, extractingMainto a dedicated file (e.g.,src/Main.tsx) and importing it here would improve the developer experience.If you'd like, create
src/Main.tsx:import React, { useMemo, useState } from 'react'; import App from './App.tsx'; import { ThemeProvider, createTheme, CssBaseline, GlobalStyles } from '@mui/material'; import { ColorModeContext } from '@/contexts/ColorModeContext'; // ... move getCSSVariable, getCSSVariableOrFallback, and Main component here export default Main;Then update
src/main.tsx:import React from 'react'; import ReactDOM from 'react-dom/client'; import Main from './Main.tsx'; import '@/styles/globals.css'; ReactDOM.createRoot(document.getElementById('root')!).render(<Main />);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/components/Layout/Header.tsx(1 hunks)src/components/Map/LayerControls.tsx(1 hunks)src/main.tsx(1 hunks)src/styles/map.css(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
src/components/Layout/Header.tsx (1)
src/contexts/ColorModeContext.tsx (1)
ColorModeContext(8-11)
src/components/Map/LayerControls.tsx (1)
src/types/map.ts (1)
LayerVisibilityMap(5-7)
src/main.tsx (1)
src/contexts/ColorModeContext.tsx (1)
ColorModeContext(8-11)
🪛 ESLint
src/components/Map/LayerControls.tsx
[error] 41-41: 'idx' is defined but never used.
(@typescript-eslint/no-unused-vars)
src/main.tsx
[error] 1-1: 'useEffect' is defined but never used.
(@typescript-eslint/no-unused-vars)
[error] 22-22: Fast refresh only works when a file has exports. Move your component(s) to a separate file.
(react-refresh/only-export-components)
🔇 Additional comments (7)
src/components/Layout/Header.tsx (1)
12-17: Well done—context-driven theme management implemented correctly.The component now reads
modedirectly fromColorModeContextand derivesisDarkModewithout maintaining local state or readinglocalStoragedirectly. This eliminates the synchronization issues flagged in previous reviews.src/main.tsx (2)
26-31: Excellent—synchronous initialization prevents theme flash.Initializing
modefromlocalStoragedirectly in theuseStateinitializer ensures the correct theme is applied on first paint, eliminating the flash of wrong theme that would occur with auseEffect-based approach. This addresses the concern from the previous review.
16-19: Great addition of fallbacks for CSS variables.The
getCSSVariableOrFallbackhelper andFALLBACKSmap ensure the theme remains functional even if CSS variables aren't loaded or defined. The validation warning on Lines 72-74 provides helpful debugging context. This addresses the previous review's concern about missing fallback values.Also applies to: 58-74
src/components/Map/LayerControls.tsx (2)
44-44: Excellent—using stable key resolves React reconciliation issue.Using
layerNameas the key instead of the array index ensures React correctly tracks each layer item when the list changes. This addresses the critical concern from the previous review about potential state mismatches during reordering or additions/removals.
55-65: Good accessibility implementation.The checkbox includes proper ARIA attributes (
aria-labelandaria-checked) throughslotProps, ensuring the control is accessible to assistive technologies.src/styles/map.css (2)
96-96: Correct usage of CSS variables in backdrop-filter.All instances of
backdrop-filter: blur(var(--app-blur))correctly wrap the CSS variable withvar(), ensuring the blur effect applies properly. This addresses the issue flagged in the previous review.Also applies to: 122-122, 144-144, 204-204
1-81: Well-structured CSS variables system.The comprehensive set of CSS variables for spacing, colors, borders, shadows, and theme-specific overrides provides a maintainable foundation for the theming system. The
[data-theme='dark']overrides ensure consistent styling across light and dark modes.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/components/Layout/Header.tsx(1 hunks)src/components/Map/LayerControls.tsx(1 hunks)src/main.tsx(1 hunks)src/styles/map.css(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/components/Map/LayerControls.tsx
🧰 Additional context used
🧬 Code graph analysis (2)
src/main.tsx (1)
src/contexts/ColorModeContext.tsx (1)
ColorModeContext(8-11)
src/components/Layout/Header.tsx (1)
src/contexts/ColorModeContext.tsx (1)
ColorModeContext(8-11)
🪛 ESLint
src/main.tsx
[error] 22-22: Fast refresh only works when a file has exports. Move your component(s) to a separate file.
(react-refresh/only-export-components)
🔇 Additional comments (1)
src/components/Layout/Header.tsx (1)
12-91: Context-driven toggle looks greatThanks for trimming the duplicated local theme state and trusting
ColorModeContextfor both the toggle and current mode; this keeps the header in sync with the rest of the app.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/main.tsx (1)
67-74: Validation warning is unreachable.
getCSSVariableOrFallbackalways returns a non-empty string (either the CSS variable or the fallback), so the check on lines 72-74 will never trigger. If you want to warn when CSS variables are missing, check the rawgetCSSVariableresult before applying fallbacks.Example refactor:
- const [primary, secondary, bgDefault, bgPaper, textPrimary, textSecondary] = vars.map( - (v) => getCSSVariableOrFallback(v, FALLBACKS[v]) - ); - - // Validate that critical variables are present - if (!primary || !bgDefault || !textPrimary) { - console.warn('Missing critical CSS variables, using fallbacks'); - } + const rawValues = vars.map(getCSSVariable); + const critical = [rawValues[0], rawValues[2], rawValues[4]]; // primary, bgDefault, textPrimary + if (critical.some(v => !v)) { + console.warn('Missing critical CSS variables, using fallbacks'); + } + const [primary, secondary, bgDefault, bgPaper, textPrimary, textSecondary] = vars.map( + (v) => getCSSVariableOrFallback(v, FALLBACKS[v]) + );src/styles/map.css (1)
245-504: Consider removing commented-out legacy styles.The old styles are preserved as reference, but if the MUI migration is complete and tested, they can be safely removed to reduce file size and maintenance burden. If you want to preserve them for historical reference, consider documenting the migration in a separate file or commit message.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/main.tsx(1 hunks)src/styles/map.css(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/main.tsx (2)
src/contexts/ColorModeContext.tsx (1)
ColorModeContext(8-11)src/components/Layout/Header.tsx (3)
isDarkMode(9-63)savedTheme(13-19)newDarkMode(22-32)
🔇 Additional comments (4)
src/main.tsx (2)
22-44: LGTM! Clean color mode management.The synchronous localStorage initialization prevents theme flash, and the toggle logic correctly updates all three locations (state, localStorage, DOM attribute). The memoized context value properly matches the ColorModeContext interface.
1-128: Header.tsx correctly uses centralized ColorModeContext—no action needed.Header.tsx has been properly updated to consume
ColorModeContextviauseContext(line 12) and callstoggleColorMode()from the context (line 16). It no longer manages separate dark mode state. The dark mode management is now centralized and consistent across the application.src/styles/map.css (2)
1-68: LGTM! Comprehensive CSS variable system.The spacing and color tokens are well-organized, with proper dark theme overrides scoped under
[data-theme='dark']. The two-tier system (light/dark) aligns well with the Main component's color mode implementation.
92-227: LGTM! Token-driven styling is consistent.All CSS variables are properly referenced with
var(), backdrop filters use the correct syntax, and hover states correctly use their respective tokens. The refactor successfully centralizes visual properties.
|
Regressions. Not ready to merge. |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/styles/map.css (1)
250-509: Consider removing commented-out code after migration validation.This file contains ~260 lines of commented-out old styling (approximately 50% of the file). While keeping old code during migration can be helpful for reference, it adds maintenance burden once the migration is complete and validated.
Since the PR description indicates that testing was performed locally in multiple browsers with no regressions, and the old code is preserved in Git history, consider removing the commented blocks in a follow-up commit after final validation:
- Lines 250-277: Old MapContainer styling
- Lines 279-429: Old MapLegend styling
- Lines 431-509: Old LayerControls styling
This will improve maintainability and reduce file size. If you prefer to keep it temporarily for additional validation, that's also reasonable given the scope of the refactor.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/main.tsx(1 hunks)src/styles/globals.css(1 hunks)src/styles/map.css(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/main.tsx (1)
src/contexts/ColorModeContext.tsx (1)
ColorModeContext(8-11)
🔇 Additional comments (7)
src/styles/globals.css (1)
47-68: LGTM! CSS variables are well-structured.The CSS variable definitions for light and dark themes are properly scoped and provide a solid foundation for the theming system. The naming convention is consistent and the values are appropriate for each mode.
src/main.tsx (3)
1-19: LGTM! Clean imports and robust helper functions.The helper functions properly handle CSS variable extraction with fallbacks, which prevents theme breakage when variables are undefined.
26-31: LGTM! Proper synchronous initialization.The lazy initialization pattern correctly prevents the flash of unstyled content by reading localStorage and setting the data-theme attribute synchronously before the first render.
34-41: LGTM! Toggle logic is correct.The toggle function properly manages the mode transition, persisting to localStorage and updating the DOM attribute for CSS variable resolution.
src/styles/map.css (3)
1-81: LGTM! Comprehensive CSS variable system.The CSS variable definitions are well-organized with clear sections for spacing, colors, and wrapper properties. The dark mode overrides are properly scoped under
[data-theme='dark'], and the naming convention is consistent throughout.
83-233: LGTM! Well-structured component styling.The component styles consistently use CSS variables from the token system, properly handle hover states, and include appropriate transitions. The styling aligns well with the theming architecture introduced in other files.
234-248: LGTM! Appropriate responsive adjustments.The media query properly handles mobile layout by repositioning the layer controls and adjusting the main content flow. The 768px breakpoint is a standard choice for tablet/mobile transitions.
| // Memoized MUI theme | ||
| const theme = useMemo(() => { | ||
| // Grab CSS variables dynamically from :root or [data-theme] | ||
| const vars = [ | ||
| '--body-bg-default', | ||
| '--color-primary', | ||
| '--color-secondary', | ||
| '--color-bg-default', | ||
| '--color-bg-paper', | ||
| '--color-text-primary', | ||
| '--color-text-secondary', | ||
| ]; | ||
|
|
||
| const FALLBACKS: Record<string, string> = { | ||
| '--body-bg-default': '#f9f9f9', | ||
| '--color-primary': '#667eea', | ||
| '--color-secondary': '#764ba2', | ||
| '--color-bg-default': '#f9f9f9', | ||
| '--color-bg-paper': 'transparent', | ||
| '--color-text-primary': '#2c3e50', | ||
| '--color-text-secondary': '#5a6c7d', | ||
| }; | ||
|
|
||
| const [primary, secondary, bgDefault, bgPaper, textPrimary, textSecondary] = vars.map( | ||
| (v) => getCSSVariableOrFallback(v, FALLBACKS[v]) | ||
| ); | ||
|
|
||
| // Validate that critical variables are present | ||
| if (!primary || !bgDefault || !textPrimary) { | ||
| console.warn('Missing critical CSS variables, using fallbacks'); | ||
| } | ||
|
|
||
| return createTheme({ | ||
| palette: { | ||
| mode, | ||
| primary: { main: primary }, | ||
| secondary: { main: secondary }, | ||
| background: { | ||
| default: bgDefault, | ||
| paper: bgPaper, | ||
| }, | ||
| text: { | ||
| primary: textPrimary, | ||
| secondary: textSecondary, | ||
| }, | ||
| }, | ||
| }); | ||
| }, [mode]); |
There was a problem hiding this comment.
Critical: Destructuring order doesn't match CSS variable semantics.
Lines 69-71 destructure the CSS variables in the wrong order. The vars array has 7 items but only 6 are destructured, and the mapping is incorrect:
primaryreceives--body-bg-defaultbut is used forpalette.primary.main(line 81)secondaryreceives--color-primarybut is used forpalette.secondary.main(line 82)bgDefaultreceives--color-secondarybut is used forpalette.background.default(line 84)
This causes the theme to use completely wrong colors.
Apply this diff to fix the destructuring:
// Grab CSS variables dynamically from :root or [data-theme]
const vars = [
- '--body-bg-default',
'--color-primary',
'--color-secondary',
'--color-bg-default',
'--color-bg-paper',
'--color-text-primary',
'--color-text-secondary',
];
const FALLBACKS: Record<string, string> = {
- '--body-bg-default': '#f9f9f9',
'--color-primary': '#667eea',
'--color-secondary': '#764ba2',
'--color-bg-default': '#f9f9f9',
'--color-bg-paper': 'transparent',
'--color-text-primary': '#2c3e50',
'--color-text-secondary': '#5a6c7d',
};
const [primary, secondary, bgDefault, bgPaper, textPrimary, textSecondary] = vars.map(
(v) => getCSSVariableOrFallback(v, FALLBACKS[v])
);Note: --body-bg-default is used in GlobalStyles (line 107) but not needed for the MUI theme palette, so I've removed it from the vars array. If you need it for other purposes, extract it separately.
🤖 Prompt for AI Agents
In src/main.tsx around lines 46 to 93, the theme code is destructuring CSS
variables in the wrong order and includes --body-bg-default (which is not used
in the MUI palette), causing colors to be assigned incorrectly; fix by removing
--body-bg-default from the vars array and replace it with the six
palette-related variables in this order: --color-primary, --color-secondary,
--color-bg-default, --color-bg-paper, --color-text-primary,
--color-text-secondary, then destructure into [primary, secondary, bgDefault,
bgPaper, textPrimary, textSecondary] so each variable maps to the correct
palette field; if --body-bg-default is still needed elsewhere (GlobalStyles),
read it separately before building the theme.
| <GlobalStyles | ||
| styles={(theme) => ({ | ||
| body: { | ||
| background: | ||
| theme.palette.mode === 'light' | ||
| ? `linear-gradient(135deg, var(--color-primary) 0%, var(--color-secondary) 100%)` | ||
| : `linear-gradient(135deg, var(--body-bg-default) 0%, var(--body-bg-default) 100%)`, | ||
| color: theme.palette.text.primary, | ||
| transition: 'var(--app-transition)', | ||
| }, | ||
| '.app-container': { | ||
| background: theme.palette.background.paper, | ||
| boxShadow: | ||
| theme.palette.mode === 'light' | ||
| ? 'var(--box-shadow-light)' | ||
| : 'var(--box-shadow-dark)', | ||
| borderRadius: 'var(--border-radius)', | ||
| overflow: 'hidden', | ||
| transition: 'var(--app-transition)', | ||
| }, | ||
| })} | ||
| /> |
There was a problem hiding this comment.
Undefined CSS variables break app-container styling.
Lines 115-116 reference --box-shadow-light and --box-shadow-dark, which are not defined in src/styles/globals.css or src/styles/map.css. This causes the box-shadow to fail silently in the respective theme modes.
Apply this diff to add the missing variable definitions to src/styles/map.css:
/* Custom variables: colors */
:root {
--border: 3px solid var(--color-primary);
--border-hover: rgba(102, 126, 234, 0.8);
--box-shadow: 0 8px 32px rgba(0, 0, 0, 0.1);
--box-shadow-hover: 0 12px 40px rgba(0, 0, 0, 0.2);
+ --box-shadow-light: 0 8px 32px rgba(0, 0, 0, 0.1);
--bg-gradient-start: rgba(255,255,255,0.95);And for dark mode:
[data-theme='dark'] {
--border: 3px solid rgba(102, 126, 234, 0.4);
--border-hover: rgba(102, 126, 234, 0.6);
--box-shadow: 0 8px 32px rgba(0,0,0,0.3);
--box-shadow-hover: 0 12px 40px rgba(0,0,0,0.4);
+ --box-shadow-dark: 0 8px 32px rgba(0,0,0,0.4);
--bg-gradient-start: rgba(26,26,46,0.95);Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
src/main.tsx around lines 101-122: the theme uses CSS variables
--box-shadow-light and --box-shadow-dark but those variables are missing,
breaking .app-container styling; open src/styles/map.css and add definitions for
--box-shadow-light (in the global/light variables, e.g., :root) and
--box-shadow-dark (in the dark-theme selector you use, e.g.,
body[data-theme="dark"] or .dark) with appropriate box-shadow values so the
GlobalStyles boxShadow references resolve for both light and dark modes; save
and verify both theme modes show the intended shadow.
| /* App Container */ | ||
| .app-container { | ||
| backdrop-filter: blur(var(--app-blur)); | ||
| } |
There was a problem hiding this comment.
Fix CSS variable dependency ordering issue.
Line 72 references --app-blur, which is defined in src/styles/map.css (line 74) rather than in this file. This creates a load-order dependency where globals.css relies on map.css being loaded first. If globals.css loads before map.css, the backdrop-filter will have no effect.
Consider one of these solutions:
Solution 1 (Recommended): Move the variable definition to this file.
Add the variable definition to the :root block in this file:
/* Custom variables: colors */
:root {
/* Light mode colors */
--body-bg-default: #f9f9f9;
--color-primary: #667eea;
--color-secondary: #764ba2;
--color-bg-default: #f9f9f9;
--color-bg-paper: transparent;
--color-text-primary: #2c3e50;
--color-text-secondary: #5a6c7d;
+
+ /* Visual effects */
+ --app-blur: 10px;
}Solution 2: Add a fallback value.
.app-container {
- backdrop-filter: blur(var(--app-blur));
+ backdrop-filter: blur(var(--app-blur, 10px));
}📝 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.
| /* App Container */ | |
| .app-container { | |
| backdrop-filter: blur(var(--app-blur)); | |
| } | |
| /* App Container */ | |
| .app-container { | |
| backdrop-filter: blur(var(--app-blur, 10px)); | |
| } |
🤖 Prompt for AI Agents
In src/styles/globals.css around lines 70 to 73, .app-container uses
var(--app-blur) which is defined in src/styles/map.css causing a load-order
dependency; to fix, add the --app-blur variable to the :root block in
globals.css (move the definition here so globals.css is self-contained), or
alternatively update the backdrop-filter to include a sensible fallback value
using var(--app-blur, 8px) if you prefer not to move the variable (recommended:
move the variable to :root in globals.css).
There was a problem hiding this comment.
Hi @barronbytes I realize this issue could have been handled better as smaller issues. Now, I notice the checkboxes are not rendering correctly. Please see my screenshots below:
If you want we can tackle checkbox component in a separate issue and merge this without MUI checkbox because this PR is huge. I would leave it up to you. Thanks for your efforts on this challenging ticket however. :)
PS: 1) Reviewers please don't merge this yet because Jonathan will work on this some more.
2) Jonathan, I am marking this PR draft until this is ready to avoid the risk of accidental merging. Hope that is ok.
3 participants
Description
Replaced custom UI components with Material UI (MUI) components. Layout properties for MUI components were moved from CSS files to TSX files. MUI light-dark theme implementation allowed for removal of manual CSS
.darkproperties. Expanded CSS reset properties. Added CSS variables for visual properties. Commented out unused code and styling at the bottom of CSS and TSX files.Type of Change
How Has This Been Tested?
Ran app locally in browsers. No regressions on Microsoft Edge, Mozilla Firefox, Google Chrome.
Checklist:
Related Issues
The Bi-monthly maintainer meeting issue document (#156) mentions several issues related to this one.
Summary by CodeRabbit
New Features
Bug Fixes
Style
Removed
✏️ Tip: You can customize this high-level summary in your review settings.