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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added Dark Mode to website #12 #17

Merged
merged 2 commits into from Oct 29, 2019
Merged

Conversation

wajeehsheikh
Copy link

Hi,

  1. Added Dark mode to the_shade_generator website.
  2. Added a button on the bottom left of the website to change to dark mode and back.

Screen Shot 2019-10-28 at 8 30 57 PM

Hope you like it.

Thank you
Wajeeh

Comment on lines +35 to +47
var options = {
bottom: '64px', // default: '32px'
right: 'unset', // default: '32px'
left: '32px', // default: 'unset'
time: '0.5s', // default: '0.3s'
mixColor: '#fff', // default: '#fff'
backgroundColor: '#fff', // default: '#fff'
buttonColorDark: '#100f2c', // default: '#100f2c'
buttonColorLight: '#fff', // default: '#fff'
saveInCookies: false, // default: true,
label: '🌓', // default: ''
autoMatchOsTheme: true // default: true
}
Copy link
Contributor

Choose a reason for hiding this comment

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

@wajeehsheikh : Nicely done. These changes are are just superficial, but it would make the code cleaner:

  1. remove the inline comments as they are unnecessary and add no value to the options object.
  2. indent the options object by 2 spaces:
var options = {
  bottom: '64px',
  right: 'unset',
  left: '32px',
  time: '0.5s',
  mixColor: '#fff'
  backgroundColor: '#fff',
  buttonColorDark: '#100f2c',
  buttonColorLight: '#fff',
  saveInCookies: false,
  label: '🌓',
  autoMatchOsTheme: true 
}

Copy link
Contributor

Choose a reason for hiding this comment

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

I see that you have included the issue number in the title of the pull request (PR), which is great, but it would be stupendous if you start referencing the issue in the PR description as well. https://help.github.com/en/github/managing-your-work-on-github/closing-issues-using-keywords

Copy link
Owner

Choose a reason for hiding this comment

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

I agree with your comment about indentation, however, I think the default values are helpful during the pull request stage. Once the code is added to the master and is included in a stable release, I'll remove them. Thanks!

<script src="js/script.js"></script>
<script src="https://cdn.jsdelivr.net/npm/darkmode-js@1.5.1/lib/darkmode-js.min.js"></script>
Copy link
Owner

Choose a reason for hiding this comment

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

Do you think it might be less confusing to add these into a new file? Going for readability here.

Copy link
Author

Choose a reason for hiding this comment

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

I think that's a great idea! I'll make the change and notify you.

Copy link
Owner

Choose a reason for hiding this comment

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

Thanks!

@varlevi
Copy link
Owner

varlevi commented Oct 29, 2019

This looks really great! The problem is that I believe it is inverting the page, which means the shades colors don't come out right. I'm pretty new to GitHub, so I'm not really sure how to add pictures, but the issue is pretty easy to replicate. Just enter the digits and generate. You can see what color is showing up. Then, press the dark mode button, and the shades change, but the digits don't.

@cdrani
Copy link
Contributor

cdrani commented Oct 29, 2019

@varlevi @wajeehsheikh : I believe that would be an easy fix. According to the docs just adding the class darkmode-ignore to the elements that render the shades should be the fix.

This allows them to stay the same color even when in dark mode.
@varlevi
Copy link
Owner

varlevi commented Oct 29, 2019

Alright. I added the darkmode-ignore class, which worked very well. Thanks!

@varlevi varlevi merged commit 53fa0b6 into varlevi:master Oct 29, 2019
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

3 participants