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 2 commits
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
27 changes: 27 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,17 @@ 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 = () => {
try {
return window?.matchMedia?.('(prefers-color-scheme: dark)')?.matches? 'dark': 'light';
}
catch {
return 'light';
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you for your time working on this issue, @aarya-16 !

This is a good path, but doing this in the app directory can bring some performance issues. I suggest you do the check using a ternary inside of the UI slice. so instead of defining the inital theme as light, we will be getting the inital one dynamically at the store level. With that we won't need to call a state update with the use effect.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll implement those changes, @marcelluscaio. I do have one question though:
Do you want me to add the event listeners to handle dynamic user system theme changes in the UI Slice as well ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Good point! I don't think we need the event listener. All we want is to assess user's initial preference. The rest is being dealt with through the settings menu. Do you see a scenario in which that would be necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The only scenario I see is if someone changes their browser or system theme settings while the website is active. But even in that case, a page reload would do the trick. So I guess I'll just make the changes without the event listener.

Copy link
Contributor

Choose a reason for hiding this comment

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

Great! I agree with you, I don't think users expect a live reload according to system settings, and we would get loose of an event listener.


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

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