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: introduce property for appearance but for now only dark is supported #4887

Merged
merged 1 commit into from
Nov 21, 2023

Conversation

benoitf
Copy link
Collaborator

@benoitf benoitf commented Nov 20, 2023

What does this PR do?

add a new setting which is hidden to toggle between light or dark mode
it's hidden because if the user is asking for light mode it will be still displayed with dark colours.

It's set to system mode but even if all the logic is there, at the end we force 'dark' mode for the system mode to not break users having wrong display of items until the light mode is completed

But it toggles on or off the dark theme support then we can use dark: prefix in the codebase to make the style being specific for the dark theme

it's like the beginning of more upcoming PRs

Screenshot/screencast of this PR

N/A as setting is hidden

What issues does this PR fix or reference?

related to #576

How to test this PR?

you can replace hidden: true by hidden:false and then you'll see the preference in preferences page

default is system so it means it'll use mode of the underlying operating system
(but default will still be mapped now to dark to not let display something broken to the user as it's the default property)

then you can inspect with 'light' or 'dark' toggle the class of the html root element (it should add dark or remove dark if you change the property)

unit tests have been added

there is a new setting which is hidden as if light mode is asked
it will be still displayed with dark theme

But it toggles on or off the dark theme support then we can use
dark: prefix in the codebase to make the style being specific
for the dark theme

related to containers#576

Signed-off-by: Florent Benoit <fbenoit@redhat.com>
@benoitf benoitf requested a review from a team as a code owner November 20, 2023 16:16
@benoitf benoitf requested review from dgolovin and lstocchi and removed request for a team November 20, 2023 16:16
@benoitf benoitf changed the title feat: introduce switch for a theme but for now only dark is supported feat: introduce property for appearance but for now only dark is supported Nov 20, 2023
@lstocchi
Copy link
Contributor

So from now on, we should go and update all bg/text color classes with dark: .... and the default value will be used in light mode?

@benoitf
Copy link
Collaborator Author

benoitf commented Nov 20, 2023

@lstocchi yes that is the plan unless using a radical different approach like https://github.com/L-Blondy/tw-colors but it won't work for what we have now as we're not using 'primary' color but a color from a palette

Copy link
Contributor

@deboer-tim deboer-tim left a comment

Choose a reason for hiding this comment

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

I had a very similar change that I was starting to test, and I'm not ashamed to say this is more thorough and sets us up well for opening the preference later. :)

My only concern is that I think we should also set the default color scheme in addition to the class, i.e.:
html.style.setProperty('color-scheme','light');
and
html.style.setProperty('color-scheme','dark');
in Appearance.svelte.

This would set the default scheme so that we only need to override where we don't match, instead of having to set styles for everything. If we do this then it's also a fix for #4888 (and we can close that part of #4826). If not, then we need to create our own dark scrollbar styling and always do things like "text-black dark:text-white" even though that would be the default.

Approving since this is an extra step beyond and I agree with what's here.

@deboer-tim
Copy link
Contributor

Yes, I think to support light we'll be changing every 'bg-x' to 'bg- dark:bg-x'. We'll need to slowly work our way through each of these anyway, so not hard to prefix existing classes.

@benoitf
Copy link
Collaborator Author

benoitf commented Nov 20, 2023

I played over the week end to see some issues (and there are ;-)

it's something that we could render:
image

Screen.Recording.2023-11-20.at.19.03.40.mov

@benoitf benoitf merged commit 7407c6f into containers:main Nov 21, 2023
9 checks passed
@podman-desktop-bot podman-desktop-bot added this to the 1.6.0 milestone Nov 21, 2023
@deboer-tim deboer-tim mentioned this pull request Nov 21, 2023
63 tasks
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.

4 participants