Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added User theme(Light/Dark) preference detection #1072

Merged
merged 4 commits into from
Oct 28, 2024
Merged

Added User theme(Light/Dark) preference detection #1072

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
77e9457
Added User theme preference detection
aarya-16 Oct 25, 2024
8c2151d
Added event listener and older browser support
aarya-16 Oct 25, 2024
0c3d824
Theme preference logic moved to UISlice
aarya-16 Oct 27, 2024
7a55997
Shifted mode ternary to a variable
aarya-16 Oct 28, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions Client/src/App.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,14 @@ import { useDispatch } from "react-redux";
import { getAppSettings, updateAppSettings } from "./Features/Settings/settingsSlice";
import { logger } from "./Utils/Logger"; // Import the logger
import { networkService } from "./main";
import { setMode } from "./Features/UI/uiSlice";

const getPreferredTheme = () => {
if(window.matchMedia && window.matchMedia('(prefers-color-scheme: dark)').matches) {
return 'dark';
}
return 'light';
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Yo dawg, let's make this function bulletproof! 🔥

The theme detection logic needs some defensive programming:

Here's a more robust implementation:

-const getPreferredTheme = () => {
-	if(window.matchMedia && window.matchMedia('(prefers-color-scheme: dark)').matches) {
-		return 'dark';
-	}
-	return 'light';
-}
+const getPreferredTheme = () => {
+  try {
+    return window?.matchMedia?.('(prefers-color-scheme: dark)')?.matches
+      ? 'dark'
+      : 'light';
+  } catch {
+    return 'light'; // Fallback for older browsers
+  }
+};

Also consider:

  1. Memoizing this function since it's used in a useEffect
  2. Adding a listener for theme changes using addListener or addEventListener
📝 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.

Suggested change
const getPreferredTheme = () => {
if(window.matchMedia && window.matchMedia('(prefers-color-scheme: dark)').matches) {
return 'dark';
}
return 'light';
}
const getPreferredTheme = () => {
try {
return window?.matchMedia?.('(prefers-color-scheme: dark)')?.matches
? 'dark'
: 'light';
} catch {
return 'light'; // Fallback for older browsers
}
};
🧰 Tools
🪛 Biome

[error] 45-45: Change to an optional chain.

Unsafe fix: Change to an optional chain.

(lint/complexity/useOptionalChain)

function App() {
const AdminCheckedRegister = withAdminCheck(Register);
const MonitorsWithAdminProp = withAdminProp(Monitors);
Expand All @@ -58,6 +66,11 @@ function App() {
}
}, [dispatch, authToken]);

useEffect(() => {
const theme = getPreferredTheme();
dispatch(setMode(theme));
},[dispatch]);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Yo, this effect needs some real-time magic! 🎵

The useEffect hook only sets the initial theme but doesn't react to system theme changes.

Here's how to make it responsive:

 useEffect(() => {
   const theme = getPreferredTheme();
   dispatch(setMode(theme));
+  
+  const mediaQuery = window?.matchMedia?.('(prefers-color-scheme: dark)');
+  const handleThemeChange = (e) => {
+    dispatch(setMode(e.matches ? 'dark' : 'light'));
+  };
+  
+  mediaQuery?.addEventListener?.('change', handleThemeChange);
+  
+  return () => {
+    mediaQuery?.removeEventListener?.('change', handleThemeChange);
+  };
 },[dispatch]);

This will keep your app's theme in sync with system preferences in real-time! 🚀

📝 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.

Suggested change
useEffect(() => {
const theme = getPreferredTheme();
dispatch(setMode(theme));
},[dispatch]);
useEffect(() => {
const theme = getPreferredTheme();
dispatch(setMode(theme));
const mediaQuery = window?.matchMedia?.('(prefers-color-scheme: dark)');
const handleThemeChange = (e) => {
dispatch(setMode(e.matches ? 'dark' : 'light'));
};
mediaQuery?.addEventListener?.('change', handleThemeChange);
return () => {
mediaQuery?.removeEventListener?.('change', handleThemeChange);
};
},[dispatch]);


// Cleanup
useEffect(() => {
return () => {
Expand Down