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: add isDarkTheme functions and composables #5698

Merged
merged 3 commits into from
Nov 6, 2024
Merged

Conversation

ShGKme
Copy link
Contributor

@ShGKme ShGKme commented Jun 13, 2024

☑️ Resolves

A copy from what we use in Talk

  • Sometimes we need to check if the dark theme is enabled
  • Currently, apps do it in different ways, and sometimes it's incorrect:
    • You should not use window.matchMedia.('(prefers-color-scheme: dark)'). It checks for the user's system theme, but Nextcloud Dark theme could be enabled even on the light system theme.
    • You should not use [data-themes*=dark] or [data-theme-dark] attributes on the body. It checks for explicitly set dark theme, but a user may use the system theme.
  • Add utils and composables to check the Nextcloud dark theme based on --background-invert-if-dark

🖼️ Screenshots

image

image

🏁 Checklist

  • ⛑️ Tests are included or are not applicable
  • 📘 Component documentation has been extended, updated or is not applicable
  • 3️⃣ Backport to next requested with a Vue 3 upgrade

@ShGKme ShGKme added enhancement New feature or request 2. developing Work in progress feature: functions composables, functions, mixins and other non-components labels Jun 13, 2024
@ShGKme ShGKme added this to the 8.13.0 milestone Jun 13, 2024
@ShGKme ShGKme self-assigned this Jun 13, 2024
@susnux
Copy link
Contributor

susnux commented Jun 13, 2024

⚠️ Requires dark theme support in docs

So you might want this a review ;)
#5656

@susnux susnux modified the milestones: 8.13.0, 8.14.0 Jun 25, 2024
@susnux susnux modified the milestones: 8.14.0, 8.15.0 Jul 4, 2024
@Antreesy Antreesy modified the milestones: 8.15.0, 8.15.1 Jul 23, 2024
@susnux susnux modified the milestones: 8.15.1, 8.15.2, 8.16.0 Jul 29, 2024
@Antreesy Antreesy modified the milestones: 8.16.0, 8.17.0 Aug 5, 2024
@susnux susnux modified the milestones: 8.17.0, 8.18.0 Aug 21, 2024
@Antreesy Antreesy modified the milestones: 8.18.0, 8.19.0 Sep 12, 2024
@susnux susnux modified the milestones: 8.19.0, 8.20.0 Sep 17, 2024
@ShGKme ShGKme force-pushed the feat/is-dark-theme branch 2 times, most recently from 4752d56 to 208a1cc Compare November 6, 2024 13:53
@ShGKme ShGKme added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Nov 6, 2024
@ShGKme ShGKme marked this pull request as ready for review November 6, 2024 14:04
Copy link
Contributor

@Antreesy Antreesy left a comment

Choose a reason for hiding this comment

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

Compared with Talk, works in Styleguids =)

docs/composables/useIsDarkTheme.md Outdated Show resolved Hide resolved
Copy link
Contributor

@susnux susnux left a comment

Choose a reason for hiding this comment

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

Only two comments on the documentation

Copy link
Contributor

@susnux susnux left a comment

Choose a reason for hiding this comment

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

Should not import with ts extension in Typescript files, while this works with bundlers, it does not for Typescript itself.

@@ -3,6 +3,7 @@
"include": ["./src/**/*.ts"],
"exclude": ["./src/**/*.cy.ts"],
"compilerOptions": {
"allowImportingTsExtensions": true,
Copy link
Contributor

Choose a reason for hiding this comment

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

This does not work, just do not import with .ts extension in Typescript files (in JS files it work).

Suggested change
"allowImportingTsExtensions": true,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This works, I missed that styleguidist uses a different config

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We have 46 places with .ts imports in this repo

Copy link
Contributor

Choose a reason for hiding this comment

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

We have 46 places with .ts imports in this repo

This works as long as it is in JavaScript files and not Typescript.
Importing with .ts extension in Typescript is invalid when emitting is enabled, as the output will be invalid.
So as long as we use a bundler everything works, but e.g. tsc would not work, because Typescript does not rewrite import extensions, so you either have to import Typescript files with .js or without any extension.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Importing with .ts extension in Typescript is invalid when emitting is enabled, as the output will be invalid.
So as long as we use a bundler everything works, but e.g. tsc would not work, because Typescript does not rewrite import extensions, so you either have to import Typescript files with .js or without any extension.

We almost never use tsc as a compiler (except notify_push) and in general, in modern TypeScript usage it's rarely used as a compiler/language and not as a static analyzer (type checker).

Specifically, in this repo, we already had @babel/typescript-preset to work with TS as a static analyzer, and only the webpack config was not correct.

src/composables/useIsDarkTheme/index.ts Show resolved Hide resolved
@ShGKme
Copy link
Contributor Author

ShGKme commented Nov 6, 2024

Fixed TS config for styleguidist (see the last commit)

@ShGKme
Copy link
Contributor Author

ShGKme commented Nov 6, 2024

Force-pushed docs fixes according to @susnux suggestions

@susnux susnux merged commit b7eeee8 into master Nov 6, 2024
19 checks passed
@susnux susnux deleted the feat/is-dark-theme branch November 6, 2024 15:20
@Antreesy
Copy link
Contributor

Antreesy commented Nov 6, 2024

backport?

@ShGKme
Copy link
Contributor Author

ShGKme commented Nov 6, 2024

/backport to next

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews enhancement New feature or request feature: functions composables, functions, mixins and other non-components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants