search

joplin / plugin-abc-sheet-music

ABC Sheet Music Plugin for Joplin
GNU General Public License v3.0
38 stars 4 forks source link

feat(theme): Force `color` to black by default, add theming option #10

Open tecc opened 4 months ago

tecc commented 4 months ago

better-theming: The CSS selector now also applies color: black to go with background-color: white. option-force-light-theme: A new option has been added that forces the colours to always be white for the foreground and black for the background, otherwise inheriting the colours. It is on by default to be as close to original behaviour as possible.

tessus commented 4 months ago

Thanks for your contribution. With PRs like these "before" and "after" screenshots help a lot to decide whether it should be merged. This is just my personal opinion, since this is Laurent's plugin, I won't be able to make and decisions anyway.

tecc commented 4 months ago

Current behaviour: bild

New behaviour, forceLightTheme = false: Inherited theme colours

New behaviour, forceLightTheme = true: Forced light theme

tessus commented 4 months ago

I like this change, although the default should probably be the previous behavior, otherwise you break people's current setup. I use the dark theme as well, but in certain situations I like the light theming look better. Having played an instrument in the past, music sheets with anything other than a light background are rather alien to me. Let's see what Laurent thinks.

tecc commented 4 months ago

The "Force light theme" option, which forces the black foreground/white background combination, is by default on.

tessus commented 4 months ago

~~Yes, but it wasn't before this PR, was it? So people who install the new version (after this PR is merged) will see the forced white background. This "breaks" their setup. When you add a feature it should not change the default behavior (except in rare circumstances)~~

I misunderstood your sentence. All is good.