Skip to content
This repository has been archived by the owner on Dec 15, 2020. It is now read-only.

add dark theme option #11

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

gravitypriest
Copy link

No description provided.

script.js Outdated
function setDarkTheme() {
$('body').addClass('dark');
$('#dark-button').hide();
$('#light-button').show();

Choose a reason for hiding this comment

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

I think the buttons visibility can be set in CSS like so:

body.dark #dark-button {
    visible: false; /* or alternatively; display:none */
}

@aaroneiche
Copy link

I like the addition of themes, I'd recommend instead of storing the property as darkThemeEnabled, store the value key as theme, then we can arbitrarily add themes to the project.

@gravitypriest
Copy link
Author

Kept both buttons visible at all times, assuming that with the addition of more themes, there will be a row of theme selection buttons in the bottom left. Maybe a dropdown if it gets too long

@aaroneiche
Copy link

Looks great! 👍

border-style: solid;
border-radius: 2px;
font-size: 14px;
font-weight: bold;

Choose a reason for hiding this comment

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

recommend an outline:none here to prevent the browser from giving the buttons an extra highlight when clicked.

@CLAassistant
Copy link

CLAassistant commented Jul 2, 2019

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

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

Successfully merging this pull request may close these issues.

3 participants