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

Joplin desktop Dark Mode #921

Merged
merged 4 commits into from Nov 7, 2018

Conversation

Projects
None yet
4 participants
@CalebJohn
Contributor

CalebJohn commented Oct 26, 2018

This adds the dark theme to Joplin desktop, there were already theme colors in place, all that was needed was to ensure that all necessary components used them.

I think this fixes #93

P.S Sorry about the file endings, I'm not sure how to adjust my editor to fix that.

@@ -59,7 +59,7 @@
let hljsScriptAdded = false;
let hljsLoaded = false;
function loadHljs(callback) {
function loadHljs(codetheme) {

This comment has been minimized.

@laurent22

laurent22 Nov 2, 2018

Owner

Please could you call the argument an "options" object with (for now) just one property "codetheme"? That way it will be possible to add more options later on, which is easier than adding more function arguments.

This comment has been minimized.

@laurent22

laurent22 Nov 2, 2018

Owner

i.e. function loadHljs(options) and below, of course, link.href = 'highlight/styles/' + options.codetheme;

codeBorderColor: '#CBCBCB',
editorTheme: "chrome",
highlightTheme: "atom-one-light.css",

This comment has been minimized.

@laurent22

laurent22 Nov 2, 2018

Owner

Any reason why it's called "highlightTheme"? In the code, we use the terms "editor" and "viewer" so for consistency I think it would be better to call it "viewerTheme", or maybe "viewerThemeCss" since it's a link to a file.

This comment has been minimized.

@CalebJohn

CalebJohn Nov 2, 2018

Contributor

This is the theme specifically for code highlighting, I wanted to avoid confusion by adding that to the name. How do you feel about codeThemeCss?

This comment has been minimized.

@laurent22

laurent22 Nov 2, 2018

Owner

Right, I missed that. Yes codeThemeCss would be good in this case.

@@ -5,8 +5,8 @@
const ipcRenderer = require('electron').ipcRenderer;
ipcRenderer.on('setHtml', (event, html) => {
window.postMessage({ target: 'webview', name: 'setHtml', data: { html: html } }, '*');
ipcRenderer.on('setHtml', (event, html, theme) => {

This comment has been minimized.

@laurent22

laurent22 Nov 2, 2018

Owner

Again please make the third parameter an "options" object with a "theme" property. Also it seems this theme only applies to code blocks? In that case, please call it "codeTheme" from the start and everywhere it's used.

@laurent22

This comment has been minimized.

Owner

laurent22 commented Nov 2, 2018

Many thanks for this pull request that will be useful to many people. It's quite a lot of work to add theme support and it seems you've covered most cases. I've added a few comments so if you could look at them and fix these issues that would be great.

@CalebJohn CalebJohn force-pushed the CalebJohn:styles branch from 0062c03 to 48aa96d Nov 2, 2018

@CalebJohn

This comment has been minimized.

Contributor

CalebJohn commented Nov 2, 2018

Nice suggestions! I added options instead of codetheme and I changed highlightTheme to codeThemeCss.

@laurent22

This comment has been minimized.

Owner

laurent22 commented Nov 2, 2018

Thanks it seems to work fine, but looks like there are a few parts that don't have the new theme yet. I'm listing them below for reference:

  • The message that ask to enter the master key:

"Some items cannot be decrypted. Set the password"

image

  • The Web Clipper options:

image

  • The Encryption options:

image

  • The sync status:

image

  • The popup to create a new notebook:

image

  • The message to disable/enable encryption (although this one is readable anyway but would be nicer if it uses the theme):

image

  • In the editor toolbar, the following buttons open popup which are not styled (or in some cases not readable this theme):
    • Note properties
    • Add Link
    • Tags
    • Alarms
@franzejr

This comment has been minimized.

franzejr commented Nov 3, 2018

This PR is great! I'm looking forward to seeing this merged!

Update app/theme.js to be more clear and more easily support addition…
…al themes

Update more files to conform to theming
@CalebJohn

This comment has been minimized.

Contributor

CalebJohn commented Nov 4, 2018

Thanks @laurent22 for the comprehensive list of missing changes. I feel a bit silly for missing a few of those. I've now updated it to cover almost all the cases but there are still 2 situations where the theme is not adhered to.

  1. Dialog boxes opened with dialogs.js
    These boxes use smalltalk.js which from what I can tell do not support on-the-fly theming. These are the popup-boxes you see that are still readable (black text on white). My recommendation is to leave them as is for now, but create an issue to note their existence so that me or someone else can change over to a new dialog system in the future.

  2. The calendar in the date picker.
    This is the same issue as above, react-datetime doesn't seem to support theming (although it says it does). Again I don't think it's that big of a deal to have a white calendar and would recommend leaving it for now.

@laurent22 laurent22 merged commit ee10610 into laurent22:master Nov 7, 2018

1 check was pending

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
@laurent22

This comment has been minimized.

Owner

laurent22 commented Nov 7, 2018

Looks great now, thanks a lot @CalebJohn! Regarding the two dialog boxes you mentioned, I think this is fine as they don't stay open for long normally so let's leave as it is.

@CalebJohn CalebJohn deleted the CalebJohn:styles branch Nov 7, 2018

@CalebJohn

This comment has been minimized.

Contributor

CalebJohn commented Nov 7, 2018

@laurent22 thanks!

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