Improve Styles, including Dark Mode #258
Conversation
Revert "mix: GlobalContext feedback 1 (no tests)." This reverts commit 97b1e72.
There was a problem hiding this comment.
@crhallberg, now that #256 is merged, there are a ton of conflicts here (since this work was based on a very early version of #256). Can you resolve the conflicts (or, if it's easier, close this PR and port the relevant changes over to a new one)?
There was a problem hiding this comment.
Thanks for the progress made here, @crhallberg! There were a few small problems that were causing test failures, so I went ahead and fixed them. This seems to be functional now, though I couldn't find a way to actually try out dark mode. See below for more detailed comments and suggestions.
| import InputLabel from '@mui/material/InputLabel'; | ||
| import MenuItem from '@mui/material/MenuItem'; | ||
| import FormControl from '@mui/material/FormControl'; | ||
| import Select, { SelectChangeEvent } from '@mui/material/Select'; |
There was a problem hiding this comment.
You're importing SelectChangeEvent but not using it.
|
|
||
| import { useGlobalContext } from "../context/GlobalContext"; | ||
|
|
||
| export default function ThemeMenu() { |
There was a problem hiding this comment.
We should add a test for this component once it is finalized and before we merge this PR.
| @@ -19,7 +19,7 @@ const ObjectButtonBar = ({ pid }: ObjectButtonBarProps): React.ReactElement => { | |||
| <ObjectStatus pid={pid} /> | |||
| <EditParentsButton pid={pid} /> | |||
| <button onClick={() => clearPidFromChildListStorage(pid)}> | |||
| <Refresh style={{ height: "14px" }} titleAccess="Refresh children" /> | |||
| <Refresh style={{ height: "20px", verticalAlign: "sub" }} titleAccess="Refresh children" /> | |||
There was a problem hiding this comment.
Should we give this a class and move the styling to CSS? I'm pretty sure the direct style here was just my laziness, but if we're going to get fancier, maybe we should do things "the right way."
|
|
||
| /* Editor */ | ||
|
|
||
| --object-status-active: #bef264; |
There was a problem hiding this comment.
I think the status colors need to be more clearly defined -- the active/inactive need to be distinct from each other so you can tell them apart at a glance, but these new colors make them look too similar:
Is there a reason we can't keep the standard red/green/yellow?
| return ( | ||
| <FormControl className="user-theme__menu"> | ||
| <InputLabel id="user-theme__label">Theme</InputLabel> | ||
| <Select |
There was a problem hiding this comment.
When it comes time to test this, take a look at the test for my bulk editor -- it's a bit tricky to test MUI select controls, but that test does it successfully. (You have to trigger a MouseUp action, if I remember correctly...)
| function systemTheme(): ThemeOption { | ||
| let defaultTheme = "light" as ThemeOption; | ||
|
|
||
| if (typeof window != "undefined" && typeof window.matchMedia != "undefined") { |
There was a problem hiding this comment.
watchMedia is not defined in server-side code, so I had to add the second part of this check to prevent test failures. We can mock the method if we want to actually test this logic (and we should, once it's ready).
| return mediaTheme; | ||
| } | ||
|
|
||
| return "system" as ThemeOption; |
There was a problem hiding this comment.
Can you return ThemeOption.system instead of "system" as ThemeOption? If that works, it seems easier to understand. (And we should use that pattern for all the theme references).
| import LogoutButton from "../components/LogoutButton"; | ||
| import ThemeMenu from "../components/ThemeMenu"; |
There was a problem hiding this comment.
You import the ThemeMenu, but you don't use it. Indeed, I don't see a theme menu anywhere in the application at this point, though I do notice that the "log out" button has moved from right-aligned to left-aligned for some reason.
| /* |
There was a problem hiding this comment.
Can/should we remove this commented-out code? Or should we consider revising the JobPaginator to use the MUI grid so we don't need this file at all? (I'm pretty sure this is only being used in JobPaginator.tsx, though maybe there are some other references I'm missing.
|
I also merged a couple of small fixes from here into dev (whitespace adjustments, removing the stray semicolon in JobPaginator) to reduce the number of files in this PR. |
Based off of the Global Context PR.