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

Fix darkmode in NC25+ #2559

Merged
merged 3 commits into from
Aug 30, 2022
Merged

Fix darkmode in NC25+ #2559

merged 3 commits into from
Aug 30, 2022

Conversation

dartcafe
Copy link
Collaborator

@dartcafe dartcafe commented Aug 26, 2022

  • change css due to new css variables since NC25
  • handle prefer-color-scheme in public polls and new apperance setting

ref nextcloud/server#32060

Signed-off-by: dartcafe <[email protected]>
@dartcafe dartcafe added this to the next milestone Aug 26, 2022
@dartcafe
Copy link
Collaborator Author

dartcafe commented Aug 30, 2022

There are some ugly tweeks, but it works for now.

@media (prefers-color-scheme: dark) {
:root {
:root, [data-theme-default] {
Copy link
Member

Choose a reason for hiding this comment

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

If new themes gets added, your app will break

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@skjnldsv Hi, thanks, but as I said in the comments: "Some dirty tweeks." 😉

The quandary is, to detect if the user opened a public page in NC<25 or an internal page in NC25 with system detect theme. Any suggestion?

Copy link
Member

Choose a reason for hiding this comment

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

Not sure I get it, what is this about? :)

Copy link
Member

Choose a reason for hiding this comment

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

If you need to know if the page is public or not, do it via PhP and check the rendered template?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

like a miracle I found body-public. But the proble are not the public pages. I have to tell if internal pages ar from NC22 or NC25 with system theme detection. The only indicator to me seems to be [data-theme-default] to detect, that this page is from NC25 with activated system theme detection.

Copy link

@nursoda nursoda Sep 3, 2022

Choose a reason for hiding this comment

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

@dartcafe Have you seen this post?!

Commenting on the last few posts: You distinguish two cases:

  1. a user is logged in – thus NC preferences shall be honored (overriding possibly different browser/system theme)
  2. no NC user session – thus NC preferences cannot and don't have to be honored, always use browser/system theme

This applies to public/link shared files pages just the same as to public polls, doesn't it?
So how does the core file sharing functionality do it?

I'm still struggling to find out what this all is about and where to start. Since I'm not a dev, maintaining twofactor_email just got even more challenging. If you're interested in mutual exchange (probably more helping me out), feel free to contact me. see privat seyfarth de olav.

Copy link
Collaborator Author

@dartcafe dartcafe Sep 3, 2022

Choose a reason for hiding this comment

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

The public pages are not the problem, because the system setting is respected since a couple of versions.
The problem are the internal pages, because we have some colors defined, which have to be set according to the theme.

And we (better I) want to keep the compatibility to NC prior to 25.

So when it comes to NC24 and before, we have to select the colors according to the default (light) theme or the dark theme. easy

Since 25, we have a third situation. The (logged in) user can have set a dark or light theme, which means the system setting is to be ignored. But if he has set the system theme, prefers-color-scheme: dark comes into play. But is has only to be applied, if the user has set this theme, not whan dark or light theme is selected.

I found out that the system theme seems to be indicated by the body attribute default-theme. Maybe a misinterpartion, but it seems to work as expected. Therefore I chosed these combinations.

logged in user dark theme set logged in user light theme set logged in user system theme set
NC 25 use [theme-dark] ignore prefers-color-scheme:dark, use [light-theme] apply prefers-color-scheme, use [default-theme]
NC 24 ignore prefers-color-scheme, use body.theme-dark ignore prefers-color-scheme, use body.theme-light not valid

Fallback then are light colors.

So it comes that @media:prefers-color-scheme: dark only has to be applied, if

  • user is not logged in OR
  • user is logged in
    • AND NC is >24
    • AND user has not set the light theme --> (current indicator [default-theme] exists)

Copy link
Collaborator Author

@dartcafe dartcafe Sep 3, 2022

Choose a reason for hiding this comment

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

To tell if a page is a public page or not can be told by the body id

  • internal pages have the id body#body-user
  • public pages have the id body#body-public

I don't know since when, but i discovered this while fiddling out the theme differences.

Copy link

Choose a reason for hiding this comment

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

…Therefore I chose these combinations.…

Shouldn't the cell "logged in user dark theme set" × "NC 25" read as follows?

ignore prefers-color-scheme:light,
use [dark-theme]

Copy link
Collaborator Author

@dartcafe dartcafe Sep 5, 2022

Choose a reason for hiding this comment

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

yes. copy&paste error. Thanks. 🙈
but only use [dark-theme], because light is default

@dartcafe dartcafe removed this from the next milestone Oct 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants