-
Notifications
You must be signed in to change notification settings - Fork 117
Added Dark Mode Feature to thecodingtrain Website #1554
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
Conversation
✅ Deploy Preview for codingtrain ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site configuration. |
Thank you again @Yashasewi I am so excited to have this, it is amazing work! (And thank you for your patience, I know it can be frustrating to have contributions linger for a long time!). I have reviewed the deploy preview and am happy with this! Does anyone have any remaining technical comments before I merge? Also, I believe I found a bug, one some pages in dark model there are still "light" sections, for example: https://deploy-preview-1554--codingtrain.netlify.app/tracks/data-and-apis-in-javascript ![]() |
other inconsistencies/bugs: (in the following screenshots kindly ignore font/text rendering issues, they are local to my machine due to weird resolution settings) Footer social links - icon issue? Track card - language/topic tags not visible (url) Track video header - section with light background, language/topic tags not visible (url) VideoSection - timestamps not visible, section with light background (url) VideoInfo + PassengerShowcasePanel - links/description/credits not visible, passenger showcase header not visible, showcase name not visible (url) (Passenger showcase is correctly rendered on the showcase page, so probably the same styles need to be used on the video pages as well) (same changes as above for "Related Challenges" section) PassengerShowcaseForm - dark text (url) |
Thank you for taking the time to review the dark mode implementation and for your valuable feedback. I appreciate you pointing out the issues and inconsistencies you've encountered. (No worries, I know these things take time! 😄) Regarding the bug you mentioned, @shiffman, where some pages in dark mode still have "light" sections (e.g., As for the other inconsistencies/bugs reported:
@dipamsen, if you could share the specific URLs where you encountered the issues, it would be extremely helpful for me to locate and fix them easily. I will work on addressing them and update the pull request accordingly. If you encounter any other inconsistencies or issues, please feel free to share them as well. |
@Yashasewi I have updated my comment to include example urls to pages on the deploy preview site. |
Regarding the footer social links, if I have the necessary permissions, I will proceed with changing the icons to new ones that support dark mode switching. This should resolve the icon visibility and styling issues in both light and dark modes. I have fixed most of the bugs found during your review. However, if you encounter any additional issues or inconsistencies, please do not hesitate to mention them. |
…e challenges page
@fturmel I had made similar local changes to address some of the remaining issues, but after you went through the entire site and made these commits, I don't need to push those local commits anymore. The improvements and fixes you've implemented cover the areas I had planned to work on. I was also testing this site on mobile devices, and the bugs I found were related to the code examples and the navigation not closing on mobile when activated. However, you have fixed both of these issues with your recent commits. I couldn't find any other issues or problems on mobile devices. I believe we can safely merge this pull request into the main branch. If someone found any issue or bug or inconsistency, please mention here. |
@Yashasewi Sorry I stole your thunder! It made sense to just power through and fix the small issues as I was QA'ing locally. I feel bad because I did spend time writing detailed comments before so you could have some direction and work it out yourself 🤦 I'll be more patient next time! And again, good job on this PR. |
@fturmel 😂 There's nothing to feel bad about at all! Working alongside an experienced contributor like yourself has been an incredible learning opportunity for me. |
Thank you, wow! I see that the checks are not passing, let me know if there's something I should do or check into! @Yashasewi I love the idea of dark mode for nature of code, but the site and its design are still quite a bit in flux so it probably would make sense to wait until it's more settled and final. Feel free to poke around the repo and look at the issues and other discussion! I welcome contributions! |
Alright, all checks are green again. There must've been an outage / glitch on Netlify's end a few days ago. |
Also, the color pair of foreground and background on the Guides page (as well as Showcase, About) may be difficult to read due to low color contrast (does not conform to WCAG AA), maybe we can use a different (lighter) shade/color theme for these pages for the light theme? |
While I am listing the changes (regressions), the social icons seems to have been changed, presumably because the original SVGs which were used could not be dynamically used for both light and dark theme. Currently icons from
P.S. Is it okay to just rotate a star and call it the nebula logo? thecodingtrain.com/src/components/Footer.js Lines 102 to 107 in 77cc494
|
@dipamsen, thanks for raising those points. Regarding the color updates, I've changed a few colors by adding opacity and reverted to the old purple color. As for the Nebula icon, you're right that it's just a rotated star design. The I agree it's not an ideal solution, but it was the best approach we could find for now to ensure the icon works well in both light and dark modes. |
@dipamsen Thanks for the review! The Nebula logo / star is a non-issue, it's the exact same shape in the end. We'll end up with the same thing if we start from the official logo and make it monochrome. I kinda like the simpler social icons without the circles personally. They're clearer and less busy that way. But if we feel strongly about retaining the old look, we can either revert and fix the SVGs, or keep the react-icons and implement our own circle wrapper. The logos are more up-to-date in react-icons, the previous SVGs probably needed a refresh anyways. @shiffman Any preferences? #1554 (comment) @Yashasewi Let me know if you need help or guidance once we have a consensus. Almost there! |
This comment was marked as off-topic.
This comment was marked as off-topic.
@Yashasewi Maybe you can open new PRs / issues for these so we can concentrate on dark mode related concerns here? |
I have no preference about the social icons and am glad to go with what is easiest to implement and maintain! I agree with @fturmel, I like your additional ideas @Yashasewi but let's work on them in separate issues / branches and focus on dark mode here! |
The current state of the PR is the easiest and simplest (react-icons without circles). The work is done, it's dark mode compatible without additional efforts, and the brand logos will be up-to-date. Are we ready to merge? @jasontheillustrator @runemadsen did you want to chime in on the feature? |
Fantastic, let's give this ~24 hours for any last comments and then I will go ahead and merge! |
Hii @dipamsen now if you check it again they look quite similar to me . But sometimes it's harder to differentiate colours for me . So please let me know if you think we need to change the colour a bit more .
And In most cases the text colour has been replaced by |
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. Apparently originally some texts were complete black whereas some other places it was a lighter opacity. (unsure whether intentional). Currently it is consistent for all copy.
Should be ready for merge! (unless there are any more comments)
Heeeeeeeere weeeeeeeeeee goooooooooooooo! Amazing work @Yashasewi! |
Dark Mode Implementation
This pull request introduces the implementation of the dark mode feature for the thecodingtrain.com website, as discussed in #1504.
Changes
theme.js
component to change CSS variables for light and dark modesImplementation Details
CSS Variables and Theming
The
theme.js
file defines two sets of CSS variables: one for the light mode and another for the dark mode. These variables control various colors and styles throughout the website, including text, backgrounds, and UI elements.The
theme.js
component manages the application of the appropriate CSS variables based on the user's preference (light or dark mode). It also persists the user's preference in localStorage for consistency across sessions.Existing Styles Updates
Existing CSS styles have been updated to reference the appropriate CSS variables, ensuring that colors and visual elements adapt seamlessly to the selected theme.
Potential Issues
Next Steps
Resolves #1504
@shiffman @runemadsen @jasontheillustrator