Skip to content

[next] feat: use nextcloud-theme-dark var for dark theme computation #6648

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 13 additions & 10 deletions src/functions/isDarkTheme/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,18 +9,21 @@
* @return {boolean} - Whether the dark theme is enabled via Nextcloud theme
*/
export function checkIfDarkTheme(el: HTMLElement = document.body): boolean {
// Nextcloud uses --background-invert-if-dark for dark theme filters in CSS
// Values:
// - 'invert(100%)' for dark theme
// - 'no' for light theme
// This is the most reliable way to check for dark theme, including custom themes
const backgroundInvertIfDark = window.getComputedStyle(el).getPropertyValue('--background-invert-if-dark')
if (backgroundInvertIfDark !== undefined) {
return backgroundInvertIfDark === 'invert(100%)'
// Check if the new clean nextcloud-theme-dark variable is set
const isNextcloudThemeDark = window.getComputedStyle(el).getPropertyValue('--nextcloud-theme-dark')
if (isNextcloudThemeDark !== undefined) {
return isNextcloudThemeDark === '1'
}

// There is no theme? Fallback to the light theme
return false
// Else we check the old ways
// 1. first priority is the data-theme-dark attribute on a parent element
// 2. second priority is the data-themes attribute on the body, aka the user setting
// 3. third priority is the system preference
// 4. lastly is the default light theme
const darkModeSystemPreference = window?.matchMedia?.('(prefers-color-scheme: dark)')?.matches
const darkModeAccountSetting = document.body.getAttribute('data-themes')?.includes('dark')
const darkModeElementOverride = el.closest('[data-theme-dark]') !== null
return darkModeElementOverride || darkModeAccountSetting || darkModeSystemPreference || false
Comment on lines +23 to +26
Copy link
Contributor

Choose a reason for hiding this comment

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

To do lazy checks and not perform x3 DOM API calls

Suggested change
const darkModeSystemPreference = window?.matchMedia?.('(prefers-color-scheme: dark)')?.matches
const darkModeAccountSetting = document.body.getAttribute('data-themes')?.includes('dark')
const darkModeElementOverride = el.closest('[data-theme-dark]') !== null
return darkModeElementOverride || darkModeAccountSetting || darkModeSystemPreference || false
if (el.closest('[data-theme-dark]') !== null) {
return true
}
if (document.body.getAttribute('data-themes')?.includes('dark')) {
return true
}
if (window.matchMedia('(prefers-color-scheme: dark)')?.matches) {
return true
}
return false

Copy link
Contributor

Choose a reason for hiding this comment

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

Or

Suggested change
const darkModeSystemPreference = window?.matchMedia?.('(prefers-color-scheme: dark)')?.matches
const darkModeAccountSetting = document.body.getAttribute('data-themes')?.includes('dark')
const darkModeElementOverride = el.closest('[data-theme-dark]') !== null
return darkModeElementOverride || darkModeAccountSetting || darkModeSystemPreference || false
const darkModeSystemPreference = () => window.matchMedia('(prefers-color-scheme: dark)')?.matches
const darkModeAccountSetting = () => document.body.getAttribute('data-themes')?.includes('dark')
const darkModeElementOverride = () => el.closest('[data-theme-dark]') !== null
return darkModeElementOverride() || darkModeAccountSetting() || darkModeSystemPreference() || false

Copy link
Contributor

Choose a reason for hiding this comment

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

// 1. first priority is the data-theme-dark attribute on a parent element
// 2. second priority is the data-themes attribute on the body, aka the user setting

Could you provide with the example where we need 2 and 1 was not enough?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ShGKme future proof. The Theming engine have been initially designed so other themes could be added or even provided on the appstore.
The [data-theme-dark] wasn't even properly discussed 🙈

}

/**
Expand Down
Loading