-
-
Notifications
You must be signed in to change notification settings - Fork 82
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(theme): add themes #1954
base: master
Are you sure you want to change the base?
feat(theme): add themes #1954
Changes from 2 commits
8b79b72
1f15f68
23a9af9
00abcd5
565b68b
84b74f4
64754a5
026228a
ac34e71
de1777e
0644e3c
f0276a7
62e08bc
963e48f
137d9c8
16ece0d
ed34640
3c11274
1cd482d
3ccbb6e
d285fa0
9296a8b
5bd29cd
daad3c3
a5f5c22
4b60f53
fb45e42
6761f78
b58cc74
ff5e0e2
66502a3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,34 @@ | ||
import { Router, static as ExpressStatic } from 'express'; | ||
|
||
import { H5PEditor } from '@lumieducation/h5p-server'; | ||
import { | ||
errorHandler, | ||
undefinedOrTrue, | ||
catchAndPassOnErrors | ||
} from '../expressErrorHandler'; | ||
|
||
/** | ||
* This router implements all Ajax calls necessary for the H5P (editor) client to work. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Description is not correct |
||
* Use it like this: server.use('/h5p', H5P.adapters.express(h5pEditor, path.resolve('h5p/core'), path.resolve('h5p/editor'))); | ||
* If you only want certain routes, you can specify this in the options parameter. | ||
* @param h5pEditor the editor object | ||
* @param h5pCorePath the path on the local disk at which the core files (of the player) can be found | ||
* @param h5pEditorLibraryPath the path on the local disk at which the core files of the editor can be found | ||
* @param routeOptions sets which routes you want and how to handle errors | ||
* @param languageOverride the language to use when returning errors. | ||
* Only has an effect if you use the i18next http middleware, as it relies on | ||
* req.i18n.changeLanguage to be present. Defaults to auto, which means the | ||
* a language detector must have detected language and req.t translated to the | ||
* detected language. | ||
*/ | ||
export default function (h5pEditor: H5PEditor): Router { | ||
const router = Router(); | ||
|
||
// get library file | ||
router.get(`/theme.css`, (req, res) => { | ||
res.setHeader('content-type', 'text/css'); | ||
res.status(200).send(h5pEditor.renderTheme()); | ||
}); | ||
|
||
return router; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -16,6 +16,7 @@ import defaultCopyrightSemanticsLanguageFile from '../assets/translations/copyri | |
import defaultMetadataSemanticsLanguageFile from '../assets/translations/metadata-semantics/en.json'; | ||
import editorAssetList from './editorAssetList.json'; | ||
import defaultRenderer from './renderers/default'; | ||
import renderTheme from './renderers/theme'; | ||
import supportedLanguageList from '../assets/editorLanguages.json'; | ||
|
||
import ContentManager from './ContentManager'; | ||
|
@@ -51,6 +52,7 @@ import { | |
ILumiEditorIntegration, | ||
ISemanticsEntry, | ||
ITemporaryFileStorage, | ||
ITheme, | ||
ITranslationFunction, | ||
IUrlGenerator, | ||
IUser | ||
|
@@ -155,8 +157,10 @@ export default class H5PEditor { | |
); | ||
} | ||
|
||
this.globalCustomStyles = | ||
this.options?.customization?.global?.styles || []; | ||
this.globalCustomStyles = this.options?.customization?.global | ||
?.styles || [ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The theme should not replace the global styles; they should be concatenated I guess There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It is not replaced but entered as the first entry in the array . There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
this.config.theme && `${this.config.baseUrl}/theme.css` | ||
]; | ||
if (this.config.customization?.global?.editor?.styles) { | ||
this.globalCustomStyles = this.globalCustomStyles.concat( | ||
this.config.customization.global?.editor.styles | ||
|
@@ -564,6 +568,10 @@ export default class H5PEditor { | |
return Promise.resolve(this.renderer(model)); | ||
} | ||
|
||
public renderTheme(theme?: ITheme): string { | ||
return renderTheme(theme || this.config.theme); | ||
} | ||
|
||
/** | ||
* Stores an uploaded file in temporary storage. | ||
* @param contentId the id of the piece of content the file is attached to; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,5 @@ | ||
import { IPlayerModel } from '../types'; | ||
import generateThemeCSS from './theme'; | ||
|
||
export default (model: IPlayerModel): string => `<!doctype html> | ||
<html class="h5p-iframe"> | ||
|
@@ -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> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The theme should be optional There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It is optional. When the theme is not defined it returns an empty string. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
<script> | ||
window.H5PIntegration = ${JSON.stringify(model.integration, null, 2)}; | ||
</script> | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Further customization options could be:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I already planed to add those.