Skip to content

feat(UI): create night mode #942

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

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

feat(UI): create night mode #942

wants to merge 6 commits into from

Conversation

a2937
Copy link
Member

@a2937 a2937 commented Aug 5, 2024

Checklist:

Closes #828

I am very very close to being finished. I just need to add a couple Cypress tests and to fix the spacing on the menu; provided the elements I added into the menu were acceptable.

@a2937 a2937 marked this pull request as ready for review August 9, 2024 00:09
@a2937 a2937 requested a review from a team as a code owner August 9, 2024 00:09
@raisedadead
Copy link
Member

Thanks for working on this @a2937!

I am requesting @huyenltnguyen to review this. Please note that it could be a while because there is a lot happening this week.

@a2937
Copy link
Member Author

a2937 commented Aug 15, 2024

Hey I just remember this pull request also resolves this at least partially. #877

@raisedadead raisedadead assigned ahmaxed and unassigned huyenltnguyen Aug 20, 2024
@raisedadead
Copy link
Member

image image

@raisedadead
Copy link
Member

Thanks for the PR. I have fixed the merge conflicts. I checked the CSS is off (both before & after fixing the conflicts) – could be dues changes we may have introduced - IDK.

In any case please check.

That said I think this PR is doing way too much than it should be? I do not think we should move to TypeScript yet. This is an old project (CJS) and relies on Node 18 but it a stable one.

Moving to TS will need a lot of work on QA.

I think you should consider simpler, smaller PRs. Pending what @ahmaxed wants to confirm:

  • Introduce a Menu similar to the main site??
  • If above is done in a PR and released add the Night Mode features? Know that it needs to consider the main site settings – we do persist the setting to user settings. Meaning if we add the Night mode features, it SHOULD NOT show/work when a user is not signed in similar to the experience on the main site.
  • TypeScript??

@raisedadead raisedadead added the status: blocked The discussion has not been finalized yet label Aug 20, 2024
@a2937
Copy link
Member Author

a2937 commented Aug 20, 2024

Yeah I did get a little excited. I really need to remember my limits.

I think it's more likely I managed to get a few things wrong when making the CSS.

As for the three things.

There is a menu. I needed a place for my button as it broke the look of the navbar. I then added a few other elements I could.

While making the PR, I couldn't remember how we checked to see if the user is in Nightmode on the main site.

The Typescript was added to make writing the Cypress tests a lot easier. I need the intellisense to make sure I was doing things right. I had not converted the main site files and had no plans to this PR.

@a2937
Copy link
Member Author

a2937 commented Aug 20, 2024

I apologize for over-complicating this PR and going outside of the scope. I will try my best to focus on one thing at a time in the future.

@a2937
Copy link
Member Author

a2937 commented Aug 20, 2024

I have removed Typescript from this PR.

@raisedadead
Copy link
Member

Hey no apologies needed. If anything we are incredibly grateful for your contributions :) We will review this soon. Thanks

@ahmaxed
Copy link
Member

ahmaxed commented Aug 22, 2024

Thanks for the hard work and looking forward to seeing this live. This feature is coming along really well. Here is some feedback to incorporate into your solution:

  • TS should be dealt with in another pr.
  • The theme sync between platforms should be postponed (needs work from the learn platform).
  • The donate button should not be folded into the menu (needs AB testing to to see if affects donations)
  • The menu item should only include curriculum, and the theme toggle. (needs AB testing to see if adding more items would effect learn sign ups)
  • the secondary color and background are used as the main pair for the /learn content. The same should happen here so the background does run into the navigation.
  • The links and theme functionality need a divider similar to learn.
  • The variables could be moved to a separate css sheet altogether.
  • The width of the open menu items in large screens should be similar to learn.

@ahmaxed
Copy link
Member

ahmaxed commented Aug 30, 2024

@a2937, the theme switcher needs to save the user preferences in localStorage so it can be applied between pages.

Also for the color to background paring, the highest contrast should be secondary font on secondary background similar to learn.

Let us know if you need clarification with the specifications or need help with the implementation.

@raisedadead
Copy link
Member

raisedadead commented Aug 31, 2024

The theme sync between platforms should be postponed (needs work from the learn platform).

Ahmad - I think this will cause a lot of unwanted confusion and support requests. We should just defer the night mode until we can tackle it from learn, and implement everything else in #942 (comment) - in other PRs if needed.

@a2937
Copy link
Member Author

a2937 commented Aug 31, 2024

the theme switcher needs to save the user preferences in localStorage so it can be applied between pages.

What key do you recommend I use to store the value?

@ahmaxed
Copy link
Member

ahmaxed commented Sep 1, 2024

The key could be called theme.
To make the themes syncable, I suggest discussing a solution in the following issue before finalizing this pull request: freeCodeCamp/freeCodeCamp#55981

@ahmaxed
Copy link
Member

ahmaxed commented Sep 26, 2024

Thanks for the hard work here, @a2937.
Please let me know if you are interested in working on freeCodeCamp/freeCodeCamp#55981 so it would unblock the news night mode.

@a2937
Copy link
Member Author

a2937 commented Sep 26, 2024

Thanks for the hard work here, @a2937. Please let me know if you are interested in working on freeCodeCamp/freeCodeCamp#55981 so it would unblock the news night mode.

I have one in the works. freeCodeCamp/freeCodeCamp#56243

However, I am not sure if I used the Redux actions correctly. That was something in the curriculum I more or less struggled with the first time and it's been a while since I messed with it.

@ghost

This comment was marked as spam.

@raisedadead
Copy link
Member

@ahmaxed and @a2937 can we make this move forward?

@a2937
Copy link
Member Author

a2937 commented Apr 14, 2025

I think I need to rebase it as there's a good bit of things that changed.

@a2937
Copy link
Member Author

a2937 commented Apr 15, 2025

Okay merge conflicts gone. I will check first thing in the morning to see if the issue has been resolved.

@raisedadead
Copy link
Member

I recommend you squash all the commits on the PR to make your life easier.

@a2937 a2937 force-pushed the feat(UI)/dark-mode branch from ea4eac2 to e4ef418 Compare April 15, 2025 14:11
@a2937
Copy link
Member Author

a2937 commented Apr 15, 2025

I decided it was in my best interest to rebase and to try to make this PR as minimal as possible.

Copy link

socket-security bot commented Apr 15, 2025

Review the following changes in direct dependencies. Learn more about Socket for GitHub.

Diff Package Supply Chain
Security
Vulnerability Quality Maintenance License
Addedcypress-localstorage-commands@​2.2.710010010078100

View full report

@raisedadead
Copy link
Member

Hey @a2937 are you still working on this?

I see these remaining issues:

Padding Issues

image

Contrast

image

Also I do not like the night mode button to be so prominent taking up space on the nav.

@ahmaxed
Copy link
Member

ahmaxed commented May 7, 2025

@a2937, thanks for the hard work here. The pr looks almost done.

We do need a dropdown menu to include the forum link, curriculum link, and a theme toggle.
Please make sure it looks similar to the /learn implementation.

The donation button stays outside of the menu button for now. I would need to AB test it to make sure it is not affecting the conversions.

All of the colors used in the UI (other than the top nav) should be flipped for the dark theme. There are some discrepancy still on the article page, author page, etc.
Let me know if you need help or clarification.

Also the cypress-localstorage-commands seems unnecessary. It could be replaced with something similar to the following: cy.window().then((win) => {
expect(win.localStorage.getItem('theme')).to.equal('dark');
});

@a2937
Copy link
Member Author

a2937 commented May 7, 2025

Oh okay. Add in a drop down menu. Clean up the margins.

I'll do my best to remove the cypress localstorage commands package. I was having trouble using normal methods.

@a2937
Copy link
Member Author

a2937 commented May 8, 2025

There are some discrepancy still on the article page, author page, etc.
Let me know if you need help or clarification.

I might need a little more insight on what colors need changed.

@raisedadead raisedadead added status: waiting review and removed status: blocked The discussion has not been finalized yet labels May 8, 2025
@ahmaxed
Copy link
Member

ahmaxed commented May 8, 2025

I might need a little more insight on what colors need changed.
I suggest doing the followings to cover all colors.

  1. start with original css files
  2. convert all of the non-variable colors to their root equivalent
  3. convert all of the applied root colors to the light palette variable equivilant
  4. redo the last conversion for the navigation colors.

That should do it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

freecodecamp.org/news/ dark mode theme
5 participants