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

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

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

Conversation

tecc
Copy link

@tecc tecc commented Feb 23, 2024

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.

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
Copy link
Member

tessus commented Feb 23, 2024

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
Copy link
Author

tecc commented Feb 24, 2024

Current behaviour:
bild

New behaviour, forceLightTheme = false:
Inherited theme colours

New behaviour, forceLightTheme = true:
Forced light theme

theming: The container element now specifies the colours in its `style` attribute. Consequently, the CSS asset specifies to inherit parent colours. It's essentially a shorter way of expressing the same thing.
@tessus
Copy link
Member

tessus commented Feb 24, 2024

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
Copy link
Author

tecc commented Feb 24, 2024

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

@tessus
Copy link
Member

tessus commented Feb 24, 2024

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.

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.

2 participants