Repository navigation
feat: add an opt-in MFE theme to the Text (HTML) XBlock [WIP] #308
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
base: main
Are you sure you want to change the base?
Changes from all commits
25702c6
a099a7f
394b71d
949f631
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 |
|---|---|---|
|
|
@@ -179,6 +179,15 @@ class HtmlBlockMixin(LegacyXmlMixin, XBlock): | |
| values=[{"display_name": _("Visual"), "value": "visual"}, {"display_name": _("Raw"), "value": "raw"}], | ||
| scope=Scope.settings, | ||
| ) | ||
| # Opt-in styling for this block. When enabled the block renders its HTML in | ||
| # a shadow root carrying the MFE theme, so page styles cannot reach the | ||
| # content and the content cannot leak styles back into the page. | ||
| include_theme = Boolean( | ||
| help=_("If enabled, this content is styled with the MFE theme and rendered in isolation."), | ||
| display_name=_("Use MFE Theme"), | ||
| default=False, | ||
| scope=Scope.settings, | ||
| ) | ||
|
|
||
| ENABLE_HTML_XBLOCK_STUDENT_VIEW_DATA = "ENABLE_HTML_XBLOCK_STUDENT_VIEW_DATA" | ||
|
|
||
|
|
@@ -190,9 +199,22 @@ class HtmlBlockMixin(LegacyXmlMixin, XBlock): | |
| def student_view(self, _context): | ||
| """Return a fragment that contains the html for the student view.""" | ||
| frag = Fragment(self.get_html()) | ||
| frag.add_css(resource_loader.load_unicode("static/css/html.css")) | ||
| frag.add_javascript("""function HtmlBlock(runtime, element){}""") | ||
| frag.initialize_js("HtmlBlock") | ||
| # The legacy html.css is not loaded for themed blocks; the theme is | ||
| # applied inside the block's shadow root instead. | ||
| if not self.include_theme: | ||
| frag.add_css(resource_loader.load_unicode("static/css/html.css")) | ||
|
|
||
| frag.add_javascript(resource_loader.load_unicode("static/js/html_block.js")) | ||
|
|
||
| # The MFE config API is only served by the LMS, so point at it | ||
| # explicitly; in Studio this falls back to the CDN defaults. | ||
| frag.initialize_js( | ||
| "HtmlBlock", | ||
| { | ||
| "include_theme": self.include_theme, | ||
| "mfe_config_api": f"{settings.LMS_ROOT_URL}/api/mfe_config/v1", | ||
| }, | ||
| ) | ||
|
Comment on lines
+207
to
+217
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. @rpenido I feel like for best backwards compatibility, it would be good to initialise the non-include-theme variant exactly as before, including the barebones HtmlBlock js implementation. Although I'm not sure about namespacing here - would the HtmlBlock js function conflict with that of other Text blocks on the same page?
Author
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. I am not sure if I agree with you on this one. Since we added more functionality to the init function (using Does that make sense? And the new 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. @rpenido yep that makes sense, thanks for checking :) I think I'm just aware that this html block is also used for the "raw html" block, which is used for arbitrary interactive (js) features - eg. the demo course feedback buttons:
|
||
| return frag | ||
|
|
||
| @XBlock.supports("multi_device") | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,145 @@ | ||
| /** | ||
| * Renders a Text (HTML) XBlock inside a shadow root so that the block is styled | ||
| * by the MFE theme without leaking styles in either direction. | ||
| * | ||
| * The block's HTML is rendered server-side, so there is nothing to "render" | ||
| * here: the job is to move the existing children into a shadow root and attach | ||
| * the theme stylesheets to it. | ||
| */ | ||
| (function () { | ||
| 'use strict'; | ||
|
|
||
| var CDN_CORE = 'https://cdn.jsdelivr.net/npm/@openedx/paragon@23/dist/core.min.css'; | ||
|
|
||
| /** | ||
| * Fetch the Paragon theme stylesheet URLs from the MFE config API. | ||
| * | ||
| * Returns `core` and `theme` arrays built from PARAGON_THEME_URLS, falling back | ||
| * to the CDN defaults for whatever the deployment does not publish. The | ||
| * deployment's own layers are preferred over the CDN ones so there is a single | ||
| * source of truth for theme URLs: Studio's editor reads the same key and builds | ||
| * its preview from it, so whatever is attached here is what is previewed there. | ||
| * | ||
| * @param {string} mfeConfigApiUrl - URL of the MFE config API. | ||
| * @returns {Promise<{core: string[], theme: string[]}>} | ||
| */ | ||
| async function getThemes(mfeConfigApiUrl) { | ||
| let themeUrls; | ||
| try { | ||
| var response = await fetch(mfeConfigApiUrl); | ||
| var mfeConfig = await response.json(); | ||
| themeUrls = mfeConfig.PARAGON_THEME_URLS || {}; | ||
| } catch (error) { | ||
| // Not fatal: the block still renders, just with the CDN defaults. | ||
| console.error('Text XBlock: failed to fetch theme URLs:', error); | ||
| themeUrls = {}; | ||
| } | ||
| var variant = themeUrls.variants && themeUrls.variants[activeVariant(themeUrls)]; | ||
| return { | ||
| core: [pickUrl(themeUrls.core) || CDN_CORE].filter(Boolean), | ||
| theme: [pickUrl(variant)].filter(Boolean), | ||
| }; | ||
| } | ||
|
|
||
| /** | ||
| * Work out which variant is active. | ||
| * | ||
| * Two shapes are published in practice: frontend-base's `Theme` | ||
| * (https://github.com/openedx/frontend-base/blob/main/types.ts) carries an optional | ||
| * `defaults` map naming the active light and dark variants, while tutor-indigo | ||
| * (https://github.com/overhangio/tutor-indigo) ships only a `variants` map with | ||
| * nothing pointing at one. So read `defaults` when it is there, and otherwise | ||
|
Comment on lines
+49
to
+51
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. @rpenido Note that the Theme interface type in frontend-base is very lenient: the
Author
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. You are right. For now, nit update to the docs here: 949f631 |
||
| * take the first variant present rather than dropping a configured theme. | ||
| */ | ||
| function activeVariant(themeUrls) { | ||
| if (themeUrls.defaults && themeUrls.defaults.light) { | ||
| return themeUrls.defaults.light; | ||
| } | ||
| var variants = themeUrls.variants || {}; | ||
| if (variants.light) { | ||
| return 'light'; | ||
| } | ||
| var names = Object.keys(variants); | ||
| return names.length ? names[0] : null; | ||
| } | ||
|
|
||
| /** | ||
| * Pick the stylesheet URL out of a `core` or `variants` entry. | ||
| * | ||
| * Entries appear either nested (`{urls: {default, brandOverride}}`, as | ||
| * tutor-indigo publishes) or flat (`{url}`). Prefer `brandOverride` so the | ||
| * deployment's theme layers on top of the CDN build, and fall back to | ||
| * `default` for configurations that publish only that. | ||
| */ | ||
| function pickUrl(entry) { | ||
| if (!entry) { | ||
| return undefined; | ||
| } | ||
| if (entry.urls) { | ||
| return entry.urls.brandOverride || entry.urls.default || entry.url; | ||
| } | ||
| return entry.url; | ||
| } | ||
|
|
||
| /** | ||
| * Attach a stylesheet inside the shadow root only. | ||
| * | ||
| * Paragon declares its custom properties on `:root`, which matches nothing | ||
| * inside a shadow tree, but they still reach the content by inheritance through | ||
| * the host element. Attaching to the document as well would restyle every | ||
| * other block sharing the page, since Paragon core carries top-level rules for | ||
| * bare element selectors. | ||
| */ | ||
| function addStylesheet(shadowRoot, url) { | ||
| var link = document.createElement('link'); | ||
| link.rel = 'stylesheet'; | ||
| link.href = url; | ||
| shadowRoot.appendChild(link); | ||
| } | ||
|
|
||
| /** | ||
| * Move the block's server-rendered children into a shadow root. | ||
| */ | ||
| function sandbox(element) { | ||
| if (element.shadowRoot) { | ||
| return element.shadowRoot.querySelector('.xblock-root'); | ||
| } | ||
|
|
||
| var shadowRoot = element.attachShadow({ mode: 'open' }); | ||
| var root = document.createElement('div'); | ||
| root.classList.add('xblock-root'); | ||
|
|
||
| // Adopt the rendered content rather than re-rendering it, so anything the | ||
| // server produced (images, anchors, embedded markup) is preserved as-is. | ||
| while (element.firstChild) { | ||
| root.appendChild(element.firstChild); | ||
| } | ||
|
|
||
| shadowRoot.appendChild(root); | ||
| return root; | ||
| } | ||
|
|
||
| /** | ||
| * XBlock view entry point. Invoked by the XBlock JS runtime as | ||
| * `HtmlBlock(runtime, element, initArgs)`. | ||
| * | ||
| * @param {Object} runtime - XBlock runtime (unused). | ||
| * @param {Element} element - The block's root element. | ||
| * @param {Object} initArgs - Data supplied by `Fragment.initialize_js`. | ||
| */ | ||
| function HtmlBlock(runtime, element, initArgs) { | ||
| if (!initArgs || !initArgs.include_theme) { | ||
| return; | ||
| } | ||
|
|
||
| var root = sandbox(element); | ||
|
|
||
| getThemes(initArgs.mfe_config_api).then(function (themes) { | ||
| [...themes.core, ...themes.theme].forEach(function (url) { | ||
| addStylesheet(root.getRootNode(), url); | ||
| }); | ||
| }); | ||
| } | ||
|
|
||
| window.HtmlBlock = HtmlBlock; | ||
| }()); | ||

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.
@rpenido I'm wondering if something like "use_mfe_theming" or an inverse "use_legacy_theming" might be more appropriate. The more I think about it, the more "include_theme" seems a little vague.
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.
"use_mfe_theme" would match the display name, and I think be more informative. :)