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

Migrate to full css-variables theming with IThemes provider #31751

Merged
merged 15 commits into from
Apr 22, 2022

Conversation

nickvergessen
Copy link
Member

@nickvergessen nickvergessen commented Mar 29, 2022

This is a first start towards a proper theme management system.
That should help us do the following:

  • Get a cleaner and more flexible approach towards Theming in general
  • Allow external apps to provide themes in the future
  • Allow the use of media-queries to select a specific theme automatically
  • Deprecate the old scssCacher method
  • Migrate away from scss WIP: move SCSS to webpack  #31743
  • Allow for much better performances in design generation and per-user design caching
  • Fix most of the guest/public/login pages that were not themed
  • Move to a declarative theming approach rather than imperative (apps handle their themes compatibility instead of having the theme provide custom css rules)

Steps

  • Implement base ITheme provider (private for now)
  • Implement theme management backend
  • Migrate Theme selection UI from accessibility to theming
  • Adapt UI
  • Implement instant design switch
  • Remove accessibility

How to review

  • Test the various ways themes can be changed per-user
    • From the user theming settings
    • With media queries
  • Try to find visual glitches
  • Experiment with Admin theming changes and colours/edits and see if everything behaves properly
    • Settings changes and preview
    • Public pages
    • Standard logged-in pages
    • Login page

Follow-ups

  • Remove scss support WIP: move SCSS to webpack  #31743
  • Fix icons generation
    • Compile with npm
    • Implement theme-switch icon colour revert (?)
  • Fix potential leftovers and deploy magic variables like
    • --background-invert-if-bright => invert the element if the main background colour is bright
    • --primary-invert-if-bright => invert the element if the primary colour is too bright
  • Make ITheme api public (?)

Peek 21-04-2022 09-37

Closes #31746

apps/theming/lib/Controller/ThemingController.php Outdated Show resolved Hide resolved
apps/theming/lib/Controller/ThemingController.php Outdated Show resolved Hide resolved
apps/theming/lib/Service/ThemeInjectionService.php Outdated Show resolved Hide resolved
apps/theming/lib/Service/ThemesService.php Outdated Show resolved Hide resolved
nickvergessen and others added 8 commits April 21, 2022 09:29
Signed-off-by: Joas Schilling <[email protected]>
Signed-off-by: John Molakvoæ (skjnldsv) <[email protected]>
Signed-off-by: John Molakvoæ (skjnldsv) <[email protected]>
Signed-off-by: John Molakvoæ <[email protected]>
Signed-off-by: John Molakvoæ <[email protected]>
@skjnldsv skjnldsv force-pushed the theming-providers branch 2 times, most recently from 29a0abb to 38e99e3 Compare April 21, 2022 11:48
@skjnldsv skjnldsv changed the title Start theming providers Migrate to full css-variables theming with IThemes provider Apr 21, 2022
@skjnldsv skjnldsv force-pushed the theming-providers branch 2 times, most recently from 2f073df to a5c4ce3 Compare April 21, 2022 12:15
@skjnldsv skjnldsv marked this pull request as ready for review April 21, 2022 12:16
@skjnldsv skjnldsv added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Apr 21, 2022
@skjnldsv

This comment was marked as resolved.

@skjnldsv skjnldsv requested a review from a team April 21, 2022 13:09
@skjnldsv skjnldsv requested a review from szaimen April 21, 2022 17:07
@szaimen
Copy link
Contributor

szaimen commented Apr 21, 2022

I'll retest in a second 👍

Copy link
Contributor

@szaimen szaimen left a comment

Choose a reason for hiding this comment

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

the dashboard app is still a bit broken in dark mode:
image

and the files app too:
image
image

Also dyslexia still does not work for me, don't know why...

and this is still not renamed:
image

But okay to me to do this in a follow-up

@szaimen
Copy link
Contributor

szaimen commented Apr 22, 2022

We should add this to breaking change of 25 (e.g. that the accessibility app is now included in theming...)

@szaimen szaimen added the pending documentation This pull request needs an associated documentation update label Apr 22, 2022
@skjnldsv skjnldsv added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Apr 22, 2022
@skjnldsv
Copy link
Member

But okay to me to do this in a follow-up

Will do! 🚀

@skjnldsv skjnldsv dismissed stale reviews from come-nc and szaimen April 22, 2022 09:19

Fixed

Copy link
Member

@skjnldsv skjnldsv left a comment

Choose a reason for hiding this comment

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

Will address design improvements in followups

@skjnldsv
Copy link
Member

integration-setup-features failure unrelated

@szaimen
Copy link
Contributor

szaimen commented Apr 22, 2022

@skjnldsv Did you find a fix for the dyslexic prpblem?

@skjnldsv
Copy link
Member

skjnldsv commented Apr 22, 2022

@skjnldsv Did you find a fix for the dyslexic problem?

Yes, will add as followup, just using the generateUrl should be enough

diff --git a/apps/theming/lib/Themes/DyslexiaFont.php b/apps/theming/lib/Themes/DyslexiaFont.php
index 50284d33b9..2629ac588c 100644
--- a/apps/theming/lib/Themes/DyslexiaFont.php
+++ b/apps/theming/lib/Themes/DyslexiaFont.php
@@ -60,23 +60,30 @@ class DyslexiaFont extends DefaultTheme implements ITheme {
        }
 
        public function getCustomCss(): string {
+               $fontPathWoff = $this->urlGenerator->linkTo('theming', 'fonts/OpenDyslexic-Regular.woff');
+               $fontPathOtf = $this->urlGenerator->linkTo('theming', 'fonts/OpenDyslexic-Regular.otf');
+               $fontPathTtf = $this->urlGenerator->linkTo('theming', 'fonts/OpenDyslexic-Regular.ttf');
+               $boldFontPathWoff = $this->urlGenerator->linkTo('theming', 'fonts/OpenDyslexic-Bold.woff');
+               $boldFontPathOtf = $this->urlGenerator->linkTo('theming', 'fonts/OpenDyslexic-Bold.otf');
+               $boldFontPathTtf = $this->urlGenerator->linkTo('theming', 'fonts/OpenDyslexic-Bold.ttf');
+
                return "
                        @font-face {
                                font-family: 'OpenDyslexic';
                                font-style: normal;
                                font-weight: 400;
-                               src: url('../fonts/OpenDyslexic-Regular.woff') format('woff'),
-                                        url('../fonts/OpenDyslexic-Regular.otf') format('opentype'),
-                                        url('../fonts/OpenDyslexic-Regular.ttf') format('truetype');
+                               src: url('$fontPathWoff') format('woff'),
+                                        url('$fontPathOtf') format('opentype'),
+                                        url('$fontPathTtf') format('truetype');
                        }
                        
                        @font-face {
                                font-family: 'OpenDyslexic';
                                font-style: normal;
                                font-weight: 700;
-                               src: url('../fonts/OpenDyslexic-Bold.woff') format('woff'),
-                                        url('../fonts/OpenDyslexic-Bold.otf') format('opentype'),
-                                        url('../fonts/OpenDyslexic-Bold.ttf') format('truetype');
+                               src: url('$boldFontPathWoff') format('woff'),
+                                        url('$boldFontPathOtf') format('opentype'),
+                                        url('$boldFontPathTtf') format('truetype');
                        }
                ";
        }

@skjnldsv skjnldsv merged commit 9a76f06 into master Apr 22, 2022
@skjnldsv skjnldsv deleted the theming-providers branch April 22, 2022 10:32
@CarlSchwan
Copy link
Member

@skjnldsv something to look into it next week, but I think this broke one integration test

@skjnldsv
Copy link
Member

@skjnldsv something to look into it next week, but I think this broke one integration test

I wondered this too, but I cannot reproduce locally 🤔

@zak39
Copy link
Contributor

zak39 commented Sep 6, 2022

Is it possible to declare sccs in a Vue template ?

@skjnldsv
Copy link
Member

skjnldsv commented Sep 6, 2022

Is it possible to declare sccs in a Vue template ?

Yes, we use scss in vue apps almost everywhere :)

@nickvergessen nickvergessen removed the pending documentation This pull request needs an associated documentation update label Nov 8, 2022
CompuCopsWelke pushed a commit to CompuCopsWelke/Neuer-Ordner that referenced this pull request Oct 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants