solarized light/dark bug fix #1081
Conversation
|
Thank you for your contribution! We will check it. |
browser/main/lib/ConfigManager.js
Outdated
| @@ -122,7 +122,7 @@ function set (updates) { | |||
| : 'default' | |||
|
|
|||
| if (newTheme !== 'default') { | |||
| editorTheme.setAttribute('href', '../node_modules/codemirror/theme/' + newTheme + '.css') | |||
| editorTheme.setAttribute('href', '../node_modules/codemirror/theme/' + newTheme.split(' ')[0] + '.css') | |||
There was a problem hiding this comment.
This fixing is just only for solarized theme.
Can you add checking that newTheme is solarized or not?
There was a problem hiding this comment.
Yes, this change is for solarized theme only. Currently, solarized dark/light is the only the theme which contains space, so this change will not cause the problem for other theme.
However, as you pointed out, if I add the newTheme is solarized or not, this makes more clear the intention of code.
Following code also doesn't check the theme, so I will also update this.
https://github.com/BoostIO/Boostnote/blob/993cb9cb0b7c72b13b1a1cba5ed26c7a29fbcdb7/browser/components/MarkdownPreview.js#L261
Thanks for reviewing!
yosmoc
force-pushed
the
solarized_bug_fix
branch
from
6b107f5 to
864c5e4
Compare
| @@ -258,7 +258,9 @@ export default class MarkdownPreview extends React.Component { | |||
| theme = consts.THEMES.some((_theme) => _theme === theme) && theme !== 'default' | |||
| ? theme | |||
| : 'elegant' | |||
| this.getWindow().document.getElementById('codeTheme').href = `${appPath}/node_modules/codemirror/theme/${theme.split(' ')[0]}.css` | |||
| this.getWindow().document.getElementById('codeTheme').href = theme.startsWith('solarized') | |||
| ? `${appPath}/node_modules/codemirror/theme/${theme.split(' ')[0]}.css` | |||
There was a problem hiding this comment.
How about using theme.substr(0, 9); instead of splitting? Looks a bit clearer to me.
There was a problem hiding this comment.
Thanks for suggestion.
Personally, I cannot see significant differences between your suggestion and original code from readable perspective. However now this code is only called for solarized dark/light theme, so I think
${appPath}/node_modules/codemirror/theme/solarized.css
is the most clear code. What do you think?
There was a problem hiding this comment.
@yosmoc Oh.. yes. Completely overlooked that.
fixup for 0bf7e8b ConfigManager is also needed to change.
solarized dark/light shares the solarized.css
yosmoc
force-pushed
the
solarized_bug_fix
branch
from
864c5e4 to
dbc0fed
Compare
sota1235
added
next release (v0.8.17)
and removed
awaiting review ❇️
|
Merged. Thank you for your contribution @yosmoc :) |
fixup commit for 0bf7e8b
Solarized light/dark shares the same css file 'solarized.css'. Split by a space and use first one, since "solarized light" and "solarized dark" are the only theme which has a space in the theme name.