-
Notifications
You must be signed in to change notification settings - Fork 0
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.
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
Add dark mode toggle #43
Conversation
…adix theme accordingly
Hey Jose! Nice job! When I set my default setting to light or dark the app opens up in the correct mode. I am having an issue where if I set my system preferences to dark mode, but then refresh the page it goes to light mode (see video). I am using Chrome. I also tested all the alerts to see the new messages, I like how they are specific to the action. Are you working on making the background transparent? I think it would be a better user experience as you don't look so 'locked out' when the alert pops up. Dark.Light.mode.toggle.mp4 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey Jose! This all looks great! Thanks for doing this! However I am having a similar problem to rachel. If I open the app it will be in dark mode, with the body and nav bar backgrounds being the dark color. However if i switch with the toggle to light mode the body changes to a white background color but the navbar background color remains the dark color unless i switch to light mode on my device. and the reverse happens as well. if my device is in light mode and i click the toggle to go to dark mode the body background color changes to the dark color but the navbar background color remains white.
It seems like the navbar is staying with the theme set by the device and not the toggle.
That was the only issue i noticed though!
Hi @jaguilar89! Nice work on addressing this issue! I like that we now have a toggle button! Speaking about enhancing user experience, I've noticed that when I toggle the button, there's a slight delay, and the theme changes faster than the toggle button switches. Is it possible to make them synchronize simultaneously? |
@jaguilar89 I wanted to note, that my Chrome browser is set to light mode, I wonder if when I refresh if this overrides the system preferences? I'm not sure if this is helpful or not but worth mentioning! |
@trushmi That's the default animation speed of the button, I've sped it up a bit so it should be little better now. |
@rachelspencer The issue with the theme changing on refresh should be fixed now! |
Hey @jaguilar89! The mode isn't changing now when I refresh! Nice! I am not seeing the nav bar change colors for both modes, they stay white with the blue text. I was also wondering if you have played around with making the modal backgrounds transparent yet? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey Jose!
Approving this now, hopefully you can alter the modal background to be transparent.
Great job on this!
@jaguilar89 Thank you for speed of the button! Now the delay is less noticeable! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me! Nice work tackling this issue @jaguilar89 !!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job implementing this feature. It worked as described in the PR. I left non-blocking comments in the code.
Sign Out | ||
</button> | ||
</Button> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion
- right now your signIn
and signOut
buttons are hard to distinguish visually. You can solve this issue a few different ways:
-
make the
SignOut
button red and theSignIn
button green -
add an icon to each button
{children} | ||
</DarkModeContext.Provider> | ||
); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job accounting for all the different use cases.
src/views/Home.jsx
Outdated
@@ -49,7 +49,7 @@ export function Home({ data, setListPath }) { | |||
required | |||
/> | |||
{user ? ( | |||
<Button className="custom-button" type="submit"> | |||
<Button className="custom-button" type="submit" variant="solid"> | |||
Create a list |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion
- right now both of this and the button to add an item look the same. I think in this case they are both acting like primary
buttons, which is fine, but perhaps consider changing the wording of this one to Share
or make it a secondary
button so the styling is slightly different.
src/index.css
Outdated
--color-border: hsla(220, 13%, 78%, 1); | ||
--color-text: var(--color-black); | ||
--color-bg-dark: var(--color-black); | ||
--color-bg-light: var(--color-white); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I spoke to Tim because I was having a hard time figuring out the transparency issues with the modal. If you comment out line 8 and 9 it resolves it, BUT it creates a whole set of other styling issues with your dark mode.
You can talk to the team to see how they feel about keeping the solid background. If there are other issues that still remain unfinished you may want focus on those rather than worry about the transparency of the background in this issue.
…ground when viewing a modal alert
feda33f
to
60652c6
Compare
Visit the preview URL for this PR (updated for commit c2f2a95): https://tcl-73-smart-shopping-list--pr43-ja-feature-dark-mode-7lo4k34h.web.app (expires Thu, 11 Apr 2024 20:09:15 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: 99eed22fcca8bd9874e77f7b7f7d1eeb554a1666 |
Description
This code adds a toggle switch for dark mode and light mode.
This came about as I was working on a problem in my previous issue where the body of the app turned white while the header and nav stayed dark, after implementing CSS changes for the sign in alerts. I figured I'd give it a go!
Update
App now recognizes users system preference and will set the theme accordingly. It will also automatically change the theme if you change your system preference in your computer settings. AC and Testing Steps have been updated.
Related Issue
N/A
Acceptance Criteria
Type of Changes
Enhancement, Accessibility
Updates
Before
After
Switch added for toggling dark mode and light mode
Testing Steps / QA Criteria
git pull
git checkout ja-feature-dark-mode-toggle
npm ci
npm start
Notes
Right now this does not automatically set the theme based on the users system/browser preference as it did before. There are methods to detect the users preference using JS and the CSS media queries we currently have, if that's a functionality we want to implement.