-
Notifications
You must be signed in to change notification settings - Fork 356
add dark mode toggle(fixes #3628) #3634
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
base: master
Are you sure you want to change the base?
Conversation
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.
Interesting idea, but we generally use a light model on the website. It is better to create a button that can change between dark and light.
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.
@VamsiVD I agree with @3011089503 it will be better to have a button that will allow user to switch to their preferred theme
I added a toggle button and made a custom CSS file, because the cyborg theme makes the text misaligned. |
Fixed an issue of the button hiding the hamburger menu when zooming in; made the button look simple and clean |
@VamsiVD WOW |
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.
First off, this looks really good! Happy to see a bit of mdwiki hacking!
One potentially breaking issue is that if someone uses the build process for the wiki (see here), it'll overwrite the changes you made on index.html
.
- Changes to the HTML should move over to
index.tmpl
. If you're feeling up for it, you could move that button into where the navigation renders inmdwiki/js/main.js
, but that is probably more work than is needed since it seems to be appearing in a decent spot. - Scripts should move into a file in
mdwiki/js/
and that file path should be added inGrintfile.js
.
Then I just have a few changes in addition to that which are not blocking. I'm a little rusty on my jQuery so feel free to let me know if there are any quirks which make changing CSS attributes necessary with jQuery, but generally I think modifying the CSS classes for elements and changing the contents of elements are the two things that should be done with jQuery. Everything else leave up to the CSS.
<meta charset="UTF-8"> | ||
<script src="https://ajax.googleapis.com/ajax/libs/jquery/1.11.1/jquery.min.js"></script> |
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.
Extra space here.
|
||
<script> | ||
$(document).ready(function () { | ||
var $html = $('html'); |
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 like you use this reference for checking if dark mode is set in code, and the body class is used in CSS. I think it'd be a little cleaner to just apply the dark-mode
class once.
style="position: fixed; top: 6px; right: 60px; z-index: 9999; border-radius: 100px; padding: 10px 10px; cursor: pointer; font-size: 18px; transition: all 0.3s ease;border:none"> | ||
<span id="toggleIcon">🌙</span> | ||
</button> | ||
<script src="https://ajax.googleapis.com/ajax/libs/jquery/1.11.1/jquery.min.js"></script> |
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.
This is a duplicate script addition, let's remove this one and keep the one in <head>
.
$html.addClass('dark-mode'); | ||
$body.addClass('dark-mode'); | ||
$navbar.removeClass('navbar-default').addClass('navbar-inverse'); | ||
$darkModeToggleBtn.css({ |
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.
Let's try to move this to the CSS.
e.preventDefault(); | ||
setDarkMode(!$html.hasClass('dark-mode')); | ||
}); | ||
$darkModeToggleBtn.hover( |
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.
This should be doable with CSS, also.
@@ -0,0 +1,57 @@ | |||
body.dark-mode { |
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.
Just a semantic comment, but since this isn't an external library let's put this in a new mdwiki/css/
directory
I’ll take care of it |
Important: Do not tick a checkbox if you haven’t performed its action.
Before Creating the Pull Request
master
– it should say "compare: branch-name".add githubusername.md
.update mi-faq.md (fixes #3264)
.After Creating the Pull Request
Description, Screenshots and/or Screencast
changed the theme to cyborg, so the website is in darkmode.
fixes #3628
Raw.Githack Preview Link
https://raw.githack.com/vamsivd/vamsivd.github.io/3628-add-dark-mode/index.html#!index.md