feat(theme): add themes #1954

JPSchellenberg · 2021-12-03T08:52:15Z

No description provided.

packages/h5p-server/src/renderers/theme.ts

sr258 · 2021-12-06T12:54:29Z

packages/h5p-server/src/renderers/player.ts

@@ -11,7 +12,7 @@ export default (model: IPlayerModel): string => `<!doctype html>
    ${model.scripts
        .map((script) => `<script src="${script}"></script>`)
        .join('\n    ')}
-
+    <style>${generateThemeCSS(model.theme)}</style>


The theme should be optional

It is optional. When the theme is not defined it returns an empty string.

On second thought I think the way to inject the theme with the global custom style is better, as it doesn‘t require changes in the client and it will also work with the iframe embed type.

sr258 · 2021-12-06T12:56:19Z

packages/h5p-examples/config.json

@@ -15,5 +15,16 @@
        "H5P.CoursePresentation": ["H5P.MathDisplay"],
        "H5P.InteractiveVideo": ["H5P.MathDisplay"],
        "H5P.DragQuestion": ["H5P.MathDisplay"]
+    },
+    "theme": {


Further customization options could be:

corner rounding other button decorations

font sizes and font faces

margins and paddings

I already planed to add those.

sr258 · 2021-12-06T12:57:13Z

packages/h5p-server/src/H5PEditor.ts

-        this.globalCustomStyles =
-            this.options?.customization?.global?.styles || [];
+        this.globalCustomStyles = this.options?.customization?.global
+            ?.styles || [


The theme should not replace the global styles; they should be concatenated I guess

It is not replaced but entered as the first entry in the array .

Really? From what I see it takes the global styles and if it is undefined it now takes the theme. They are not combined. The && doesn‘t work here, as this only works in React.

sr258 · 2021-12-06T12:59:12Z

packages/h5p-server/src/types.ts

+    fontColor: string;
+    disabledFontColor: string;
+    backgroundColor: string;
+    paperBackgroundColor: string;


"paper" is a Material UI term; is there a more generic term?

secondaryBackground?

sr258 · 2021-12-06T13:00:13Z

It would also make sense to accomodate scenarios in which you have a per-content object theme that is configurable by the content author.

sr258 · 2021-12-22T16:16:10Z

packages/h5p-express/src/ThemeRouter/ThemeExpressRouter.ts

+} from '../expressErrorHandler';
+
+/**
+ * This router implements all Ajax calls necessary for the H5P (editor) client to work.


Description is not correct

sr258 · 2021-12-22T16:18:42Z

packages/h5p-server/src/types.ts

@@ -2089,3 +2092,24 @@ export interface ILockProvider {
        options: { timeout: number; maxOccupationTime: number }
    ): Promise<T>;
 }
+
+export interface ITheme {
+    themeUrl: string;


This property is used nowhere else? What is its function?

sr258 · 2022-02-26T17:31:23Z

There are still details in content type editors that aren't styled: (didn't look at all of them yet)

sr258 · 2022-02-28T07:48:55Z

I've fixed most of the issues above in the editor. There's mostly work to be done in the course presentation editor (it's various submenus).

I've noticed that it seems like a bad idea to override the color property of the body as this has many side effects. H5P is so complex that it's difficult to predict where there might be a panel with a hardcoded background color, so setting the color globally is dangerous as it can easily lead to situations in which there's white on white. It would probably be better to set the color in a more restricted way.

I haven't really looked at the player yet.

sr258 · 2022-06-17T14:48:07Z

I've reviewed the state of the theme in the player and I think the approach of the PR isn't working out. Nearly all content types are broken in some way. I've reviewed about 40% (results below)

I suggest this:

We have a list of content types that are supported (separate for editor and player).
There are two themes, one for editor, one for player
The CSS of the theme is never global but always scoped to the content type (in the player, and as far as possible also in the editor)
The H5PEditor and H5PPlayer object decide whether to add a theme to the UI based on the content type
There's a callback that implementations can pass in to disable themes for content (so that it is a setting for the content object)
We go through the list of content types one by one and write SCSS for it, checking every feature and option. We avoid duplication using SCSS imports and mixins.

@JPSchellenberg @jankapunkt What do you think of this approach?

jankapunkt · 2022-06-17T15:00:58Z

Wow, @sr258 you made an effort here. From my perspective this sounds good, especially with the scoping. We also use scss but I wonder if there are people who use just css or things like tailwind. Can they still use the theming then?

sr258 · 2022-06-17T15:02:36Z

Yes, the theme builder compiles it to css.

JPSchellenberg added 2 commits December 3, 2021 00:56

feat(theme): add initial theming functionality

8b79b72

feat(theme): add theming options

1f15f68

JPSchellenberg self-assigned this Dec 3, 2021

JPSchellenberg added the [type] feature Changes that introduce a new feature (resulting in minor-version-bump) label Dec 3, 2021

sr258 reviewed Dec 6, 2021

View reviewed changes

packages/h5p-server/src/renderers/theme.ts Show resolved Hide resolved

sr258 reviewed Dec 6, 2021

View reviewed changes

JPSchellenberg added 7 commits December 18, 2021 17:05

Merge branch 'master' into feat/theme

23a9af9

refactor(theme): remove redundant styles

00abcd5

feat(theme): add further customisation options

565b68b

refactor(secondaryBackground): rename paperBackgroundColor

84b74f4

feat(theme): add further style options

64754a5

Merge branch 'master' into feat/theme

026228a

fix(h5pconfig): load optional settings

ac34e71

sr258 reviewed Dec 22, 2021

View reviewed changes

sr258 added 4 commits February 26, 2022 17:40

refactor: minor issues

de1777e

refactor: more elegant theme injection in player

0644e3c

refactor: better theme injection in editor

f0276a7

feat: added styles for h5p content hub

62e08bc

sr258 added 6 commits February 26, 2022 19:31

test: fix test

963e48f

refactor: removed hardcoded theme url

137d9c8

refactor: cleaned up theme file

16ece0d

feat: added sass theme builder

ed34640

refactor: multi-file scss

3c11274

refactor: improved scss

1cd482d

sr258 added 8 commits February 27, 2022 16:29

refactor: scss restructuring editor + fixes

3ccbb6e

refactor: improve theme

d285fa0

fix: metadata theme

9296a8b

fix: better theme

5bd29cd

feat: branching scenario editor

daad3c3

fix: improved theme

a5f5c22

refactor: more improvements to editor theme

4b60f53

refactor: theme improvements

fb45e42

sr258 added 4 commits June 17, 2022 15:44

Merge remote-tracking branch 'origin/master' into feat/theme

6761f78

test: updated snapshot

b58cc74

refactor: fixed upload test

ff5e0e2

chore: update package lock

66502a3

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(theme): add themes #1954

feat(theme): add themes #1954

JPSchellenberg commented Dec 3, 2021

sr258 Dec 6, 2021

JPSchellenberg Dec 18, 2021

sr258 Dec 18, 2021

sr258 Dec 6, 2021

JPSchellenberg Dec 18, 2021

sr258 Dec 6, 2021

JPSchellenberg Dec 18, 2021

sr258 Dec 18, 2021

sr258 Dec 6, 2021

JPSchellenberg Dec 18, 2021

sr258 Dec 18, 2021

sr258 commented Dec 6, 2021

sr258 Dec 22, 2021

sr258 Dec 22, 2021

sr258 commented Feb 26, 2022

sr258 commented Feb 28, 2022

sr258 commented Jun 17, 2022

jankapunkt commented Jun 17, 2022

sr258 commented Jun 17, 2022

feat(theme): add themes #1954

Are you sure you want to change the base?

feat(theme): add themes #1954

Conversation

JPSchellenberg commented Dec 3, 2021

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

sr258 commented Dec 6, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sr258 commented Feb 26, 2022

sr258 commented Feb 28, 2022

sr258 commented Jun 17, 2022

jankapunkt commented Jun 17, 2022

sr258 commented Jun 17, 2022