Add new ThemeSwitcher logic #9698

pascalwengerter · 2023-09-17T14:56:53Z

Description

Overhaul of theme loading (used to be object key-value instead of an array of themes, which seems more desireable)-

Motivation and Context

Supersede && finish some draft PRs, take advantage of upcoming major release

Open tasks:

Implement
Test
Document

packages/web-runtime/themes/owncloud/theme.json

packages/web-runtime/src/components/Topbar/TopBar.vue

packages/web-pkg/src/composables/piniaStores/theme.ts

pascalwengerter · 2023-12-05T20:32:42Z

packages/web-app-importer/package.json

@@ -16,6 +16,7 @@
  "peerDependencies": {
    "@ownclouders/design-system": "workspace:*",
    "@ownclouders/web-client": "workspace:*",
-    "@ownclouders/web-pkg": "workspace:*"
+    "@ownclouders/web-pkg": "workspace:*",
+    "pinia": "^2.1.7"


Perhaps we should re-export the required part (storeToRefs) from web-pkg?

IMO it's fine to have pinia as peerDependency in most of our packages since that will probably be the case anyway after fully switching to it.

pascalwengerter · 2023-12-05T20:33:06Z

packages/web-runtime/src/store/config.ts

-    privacyUrl: '',
-    imprintUrl: '',
-    accessDeniedHelpUrl: '',
+    logoutUrl: '', // Fixme: why was this missing before?


Was this a conscious omission?

Don't think so, I feel like we sometimes forget adding new configs here 😅

pascalwengerter · 2023-12-05T20:34:10Z

packages/web-pkg/src/composables/piniaStores/theme.ts

+    const currentLocalStorageThemeName = useLocalStorage(themeStorageKey, theme.name)
+    currentLocalStorageThemeName.value = currentTheme.value.name
+
+    // TODO: Shouldn't we loop over all designTokens and set them?


@kulmann do you remember if we only applied the colorPalette on purpose instead of applying all designTokens?

don't remember it. but yes, we should loop over all design tokens... otherwise one theme would leak into the other, newly selected theme.

It technically still could since we're not resetting everything, but at least with the change I just pushed one isn't left wondering why the e.g. fontsize never changes even though a theme might re-define it 😋

changelog/unreleased/change-theme-handling

packages/web-runtime/src/App.vue

kulmann · 2023-12-05T21:27:08Z

packages/web-pkg/src/composables/piniaStores/theme.ts

+    const currentLocalStorageThemeName = useLocalStorage(themeStorageKey, theme.name)
+    currentLocalStorageThemeName.value = currentTheme.value.name
+
+    // TODO: Shouldn't we loop over all designTokens and set them?


don't remember it. but yes, we should loop over all design tokens... otherwise one theme would leak into the other, newly selected theme.

packages/web-pkg/src/composables/piniaStores/theme.ts

pascalwengerter · 2023-12-06T07:53:30Z

packages/web-runtime/src/helpers/theme.ts

    }
    const theme = await response.json()
-    return { web: theme.web || theme, common: theme.common || {} }
+
+    if (WebThemeConfigSchema.safeParse(theme).success) {


@dschmidt is this roughly what you had in mind?

packages/web-app-importer/src/extensions.ts

JammingBen · 2023-12-07T11:45:50Z

packages/web-app-importer/package.json

@@ -16,6 +16,7 @@
  "peerDependencies": {
    "@ownclouders/design-system": "workspace:*",
    "@ownclouders/web-client": "workspace:*",
-    "@ownclouders/web-pkg": "workspace:*"
+    "@ownclouders/web-pkg": "workspace:*",
+    "pinia": "^2.1.7"


IMO it's fine to have pinia as peerDependency in most of our packages since that will probably be the case anyway after fully switching to it.

JammingBen · 2023-12-07T12:25:48Z

packages/web-runtime/src/store/config.ts

-    privacyUrl: '',
-    imprintUrl: '',
-    accessDeniedHelpUrl: '',
+    logoutUrl: '', // Fixme: why was this missing before?


Don't think so, I feel like we sometimes forget adding new configs here 😅

JammingBen · 2023-12-07T12:30:27Z

docs/getting-started.md

- `options.imprintUrl` Specifies the target URL for the imprint link valid for the ocis instance in the account menu.
- `options.privacyUrl` Specifies the target URL for the privacy link valid for the ocis instance in the account menu.


I guess the config for this has been removed intentionally? Because then we need to remove it in oCIS as well.

Yes, we should. The way to go is the theme, so that all clients can use the URLs. Config is only for web.

JammingBen · 2023-12-07T12:38:16Z

packages/web-runtime/src/container/bootstrap.ts

-    currentThemeName.value = useDefaultThemeName()
-  }
-  await store.dispatch('loadTheme', { theme: web[unref(currentThemeName)] || web.default })
+  const themeStore = useThemeStore()


Also not in an injection context here... I know we currently don't have an alternative, but one day that might become a problem 😬 @dschmidt Maybe also something to consider for a future refactoring.

packages/web-runtime/src/composables/head/useHead.ts

…eming logic

…ction and dedicated client key

AlexAndBear · 2023-12-11T14:15:29Z

packages/web-app-files/src/views/FilesDrop.vue

@@ -89,6 +91,9 @@ export default defineComponent({
    const { getInternalSpace } = useGetMatchingSpace()
    useUpload({ uppyService })

+    const { currentTheme } = storeToRefs(themeStore)
+    const themeSlogan = computed(() => currentTheme.value.common.slogan)


Does this need to be computed, as storeToRefs builds already a reactive object, or does this not apply to nested objects?

As far as I understand it: It's not necessary in this case. currentTheme is already a Ref because of storeToRefs. Meaning the consuming code could go directly via unref(currentTheme).common.slogan.

It doesn't hurt to have a computed here though, I guess it's used as a "shortcut".

sonarcloud · 2023-12-11T14:38:20Z

SonarCloud Quality Gate failed.

0 Bugs
0 Vulnerabilities
0 Security Hotspots
2 Code Smells

61.0% Coverage
0.0% Duplication

Catch issues before they fail your Quality Gate with our IDE extension SonarLint

kulmann

nice work 💪

kulmann · 2023-12-11T14:49:44Z

packages/web-runtime/src/components/Topbar/UserMenu.vue

-      )
-    })
+
+    const imprintUrl = computed(() => themeStore.currentTheme.common.urls.imprint)


For the record, we might need to re-introduce setting the imprint and privacy urls via config.json for short term deployment reasons.

to be solved in a followup if needed

New theme structure --------- Co-authored-by: Jannik Stehle <[email protected]>

Update theming docs after #9698

AlexAndBear previously requested changes Oct 30, 2023

View reviewed changes

packages/web-runtime/themes/owncloud/theme.json Outdated Show resolved Hide resolved

This was referenced Nov 7, 2023

Theme switcher for more themes #9396

Closed

Theme names #9401

Closed

pascalwengerter force-pushed the change/rework-theme-logic branch from b233efc to 93a9bd0 Compare November 7, 2023 17:37

dschmidt reviewed Nov 7, 2023

View reviewed changes

packages/web-runtime/src/components/Topbar/TopBar.vue Outdated Show resolved Hide resolved

pascalwengerter force-pushed the change/rework-theme-logic branch from 9e8b5f0 to 8ad955a Compare November 7, 2023 18:58

pascalwengerter commented Nov 7, 2023

View reviewed changes

packages/web-pkg/src/composables/piniaStores/theme.ts Outdated Show resolved Hide resolved

pascalwengerter force-pushed the change/rework-theme-logic branch from 3df7eb7 to be849e0 Compare November 14, 2023 13:12

pascalwengerter requested a review from kulmann November 17, 2023 12:36

pascalwengerter force-pushed the change/rework-theme-logic branch 2 times, most recently from f3abe3f to acced1a Compare December 5, 2023 20:22

pascalwengerter commented Dec 5, 2023

View reviewed changes

kulmann reviewed Dec 5, 2023

View reviewed changes

dschmidt reviewed Dec 5, 2023

View reviewed changes

packages/web-pkg/src/composables/piniaStores/theme.ts Outdated Show resolved Hide resolved

pascalwengerter commented Dec 6, 2023

View reviewed changes

JammingBen reviewed Dec 7, 2023

View reviewed changes

pascalwengerter added 12 commits December 8, 2023 10:38

Move themes to separate pinia store

b14361e

Move theme pinia store to web-pkg

b916700

Next iteration

efb42cb

Add themeSwitcher for more >2 themes as OcDrop, further streamline th…

9493526

…eming logic

Next iterations

81f5735

Rebase and update theme logic handling

d6057f6

Fix some pinia reactivity issues

e5f9e0e

Address code review

8200545

Restructure theme.json and loading to reintroduce top-level common se…

44854b6

…ction and dedicated client key

Use zod to parse theme

8831f15

Fix logical error and improve typing in theme validation

38b1017

Log ZOD error when validating theme

84d34e0

JammingBen added 2 commits December 11, 2023 13:22

fix: favicon meta tag

58dd4ec

fix: app banner zod

ccf3493

JammingBen mentioned this pull request Dec 11, 2023

[full-ci] fix: updating logo with new theme structure owncloud/ocis#7930

Merged

5 tasks

JammingBen force-pushed the change/rework-theme-logic branch from b72b176 to ccf3493 Compare December 11, 2023 12:49

test: allow setting appBanner theme config in tests

13e5308

AlexAndBear reviewed Dec 11, 2023

View reviewed changes

JammingBen added 2 commits December 11, 2023 15:24

fix: theme store definition

2513032

feat: use palette icon

60e0822

kulmann approved these changes Dec 11, 2023

View reviewed changes

pascalwengerter requested a review from AlexAndBear December 11, 2023 15:03

kulmann merged commit 4da807e into owncloud:master Dec 11, 2023
3 of 4 checks passed

ownclouders pushed a commit that referenced this pull request Dec 11, 2023

Add new ThemeSwitcher logic (#9698)

aa3cd28

New theme structure --------- Co-authored-by: Jannik Stehle <[email protected]>

kulmann mentioned this pull request Dec 11, 2023

[docs] Theming dev docs outdated #10146

Closed

pascalwengerter added a commit to pascalwengerter/web that referenced this pull request Dec 12, 2023

Update theming docs after owncloud#9698

86a297a

pascalwengerter added a commit to pascalwengerter/web that referenced this pull request Dec 12, 2023

Update theming docs after owncloud#9698

89f3109

kulmann mentioned this pull request Dec 12, 2023

One theme(ing logic) to rule them all 🎨 #9939

Closed

This was referenced Dec 12, 2023

Create composable for current theme #9885

Closed

Provide nicer-to-read names for themes #9403

Closed

micbar mentioned this pull request Dec 12, 2023

[full-ci] docs: prepare changelog, bump version owncloud/ocis#7951

Merged

12 tasks

This was referenced Dec 12, 2023

theme.json – iterate in the common section #8424

Closed

Complex theming #2404

Closed

AlexAndBear pushed a commit that referenced this pull request Dec 13, 2023

Add new ThemeSwitcher logic (#9698)

2c62b22

New theme structure --------- Co-authored-by: Jannik Stehle <[email protected]>

JammingBen mentioned this pull request Dec 14, 2023

test: re-enable e2e test for changing logo #10168

Merged

5 tasks

kulmann added a commit that referenced this pull request Dec 14, 2023

Merge pull request #10151 from pascalwengerter/docs/update-theming

eafef76

Update theming docs after #9698

ownclouders pushed a commit that referenced this pull request Dec 14, 2023

Merge pull request #10151 from pascalwengerter/docs/update-theming

b0aeac4

Update theming docs after #9698

micbar mentioned this pull request Dec 20, 2023

Release 5.0.0 owncloud/ocis#7940

Closed

71 tasks

pascalwengerter mentioned this pull request Jan 7, 2024

Provide light and dark logos in common section #8855

Closed

pascalwengerter deleted the change/rework-theme-logic branch January 18, 2024 18:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add new ThemeSwitcher logic #9698

Add new ThemeSwitcher logic #9698

pascalwengerter commented Sep 17, 2023

pascalwengerter Dec 5, 2023

JammingBen Dec 7, 2023

pascalwengerter Dec 5, 2023

JammingBen Dec 7, 2023

pascalwengerter Dec 5, 2023

kulmann Dec 5, 2023

pascalwengerter Dec 5, 2023

kulmann Dec 5, 2023

pascalwengerter Dec 6, 2023

JammingBen Dec 7, 2023

JammingBen Dec 7, 2023

JammingBen Dec 7, 2023

kulmann Dec 8, 2023

JammingBen Dec 7, 2023

AlexAndBear Dec 11, 2023

JammingBen Dec 11, 2023

sonarcloud bot commented Dec 11, 2023

kulmann left a comment

kulmann Dec 11, 2023

		- `options.imprintUrl` Specifies the target URL for the imprint link valid for the ocis instance in the account menu.
		- `options.privacyUrl` Specifies the target URL for the privacy link valid for the ocis instance in the account menu.

Add new ThemeSwitcher logic #9698

Add new ThemeSwitcher logic #9698

Conversation

pascalwengerter commented Sep 17, 2023

Description

Motivation and Context

Open tasks:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarcloud bot commented Dec 11, 2023

kulmann left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment