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.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement dark mode switch #2323

Merged
merged 10 commits into from May 13, 2024
Merged

Implement dark mode switch #2323

merged 10 commits into from May 13, 2024

Conversation

oyenuga17
Copy link
Contributor

Hi @sabine so i played around with this over the weekend and I managed to pull it off.
Here, is a video of the implementation. Please let me know what you think 馃槉

Copy link
Collaborator

@sabine sabine left a comment

Choose a reason for hiding this comment

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

Hey @oyenuga17! Thanks for the contribution! This looks very nice.

Here's a few issues:

  • ESC key cannot be used to close the modal
  • clicking outside the modal does not close the modal

I think that having a HTML/JS/CSS modal here is probably too complex for what is needed here. It will be much simpler to use the browser's built in confirm() method (see https://developer.mozilla.org/en-US/docs/Web/API/Window/confirm?retiredLocale=de) to obtain confirmation from the user.

Alternatively, we can update the privacy policy to list that using the dark/light mode switch will use LocalStorage to remember the user's setting.

@oyenuga17
Copy link
Contributor Author

hmm... you're right @sabine, might be hard to achieve a fully accessible modal using this my approach. I'll try the window.confirm method since that's the closest thing to what we're trying to achieve.

@oyenuga17
Copy link
Contributor Author

@sabine I just mad the change. Please help review again

@oyenuga17 oyenuga17 requested a review from avsm as a code owner May 13, 2024 15:28
@sabine
Copy link
Collaborator

sabine commented May 13, 2024

Thanks @oyenuga17, I did some refactoring with the intent to improve code clarity. 鉂わ笍 Let's merge this now!

@sabine sabine merged commit b8808fe into ocaml:main May 13, 2024
2 of 3 checks passed
@oyenuga17
Copy link
Contributor Author

Thank you @sabine 鉂わ笍 馃殌

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

Successfully merging this pull request may close these issues.

None yet

2 participants