feat: adds support for loading external theme CSS for MFEs #440
Conversation
adamstankiewicz
changed the title
src/react/AppProvider.jsx
Outdated
| if (!appThemeState?.isThemeLoaded) { | ||
| return null; | ||
| } |
There was a problem hiding this comment.
[inform] Don't render the app until the core theme CSS and theme variant(s) CSS is loaded. This helps avoid a Flash of Unstyled Content (FoUC) on initial page load.
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #440 +/- ##
==========================================
- Coverage 83.22% 79.05% -4.18%
==========================================
Files 40 48 +8
Lines 1073 1351 +278
Branches 197 283 +86
==========================================
+ Hits 893 1068 +175
- Misses 168 251 +83
- Partials 12 32 +20 ☔ View full report in Codecov by Sentry. |
|
I don't know enough about MFEs to comment about the details of this change, but what I understand is that it will require just two settings to customise the CSS, which is great. One thing, though: this new extension mechanism will only be as good as its documentation. So I urge you to spend some time to document it as best you can. Actually, I suggest that you write the docs before finalizing the implementation, in README-driven-development style. For instance, you should describe about Tutor users can create and integrate new themes. Reading the docs will allow us to comment on this PR in a much more informed way. Also, writing the docs will help you realize whether the new extension mechanism integrates well with the workflows of platform administrators and Open edX developers. |
|
I have a couple of question,
|
|
@adamstankiewicz, some stuff that came up out of a high-level discussion with tCRIL engineering:
These aren't necessarily formal objections by tCRIL, by the way. Just adding some stuff to keep in mind. :) |
|
@arbrandes Thanks for sharing the input!
If we did end up using/recommending jsDelivr as an off-the-shelf free CDN supporting versions already aligned with NPM/Github releases, they guarantee any file from any version of the NPM package will remain accessible forever. They even support version fallback (say, a file is deleted in v1.0.2 that was previously there in v1.0.1, if you try to access that now-deleted file on v1.0.2, jsDelivr is smart enough to fallback to the version of the file in v1.0.1.
I was actually just chatting with @brian-smith-tcril about this a bit, too 😄 generally, though, +1 to the input and I'd extend it to say that we might be able to fallback to the installed CSS within the webpack bundle should the external CSS fail to load for whatever reason so at least the app renders with some styles, even if those styles may be from a slightly older version than what might be upstream in the CDN, for example. I believe this latter bit around falling back to locally installed CSS addresses at the third feedback item as well regarding performance? If the external CSS files are on a CDN(e.g., jsDelivr), the first few requests to a particular version of the theme may take a bit longer but subsequent requests to the same versions/files in the future would be cached and nearly instantaneous to load. If it errors, falling back to slightly out of date CSS would at least still make the application functional. See https://www.jsdelivr.com/documentation for more details on how jsDelivr is production-ready (e.g., if Cloudflare goes down, jsDelivr instantly reverts to another CDN provider instead, has support for China, etc.). |
|
@ghassanmas's questions above were largely answered in the Frontend Working Group meeting earlier today. To recap though:
The core theme CSS ( Assuming applications don't have their own custom styles, applications indeed could not have any custom SCSS/CSS files at all, only relying on the external theme CSS. If applications do have their own custom styles (hopefully not overriding Paragon styles!), By doing so, the application
The externally hosted, already-compiled theme CSS will need to versioning according to semantic versioning and aligned to NPM/Github releases. In our current MFE architecture, MFEs would use the latest release for the major version compatible with the installed |
@regisb Noted! Yes, documentation is definitely a critical component of this project to move from current state of theming to runtime theming via design tokens. While there is no specific documentation to this frontend-platform implementation quite yet, the existing ADR proposing a related approach helped give some motivation/context for this PR. That said, I've taken an action item to:
Also worth noting there is a separate documentation effort needed for how to do the Paragon theming with design tokens and CSS variables 😄 |
|
I can't comment on the implementation, but the overall approach, and the concept of the core and light css variable bundles seem very appropriate to me. Thanks for getting this done. |
|
Thanks for your answer @adamstankiewicz! I think that the extension docs should live close to the rest of the developer docs: https://docs.openedx.org/en/latest/developers/index.html |
Considering that it's now possible to have richer JSON-based config thanks to the config API. (i.e. the config API can return a JSON for any setting). Perhaps this code can be made more generic to load any kind of theme? i.e. the config api can return a structure like: Which is the structure being built here: If the config API isn't enabled, then it can continue to fall back to the |
|
@xitij2000 Great suggestion! Yeah, agreed this should prefer any runtime config settings, and otherwise fall back to the regular ol' environment variables. I can't imagine it'd be too much of a lift to support the runtime config use case, to use an already created object structure representing the theme core/variant URLs, if it exists. I'm not immediately sure what that implementation might look like as I'm not all that familiar with the config API outside of how frontend-platform calls it; is this something you might want to take a stab at (feel free to put some pseudocode or otherwise together as a PR to this one!)? Out of curiosity... I'm not actually sure, is the runtime config used in production by anyone at this point (AFAIK it's not being used by 2U/edX yet)? |
|
We are already using the runtime config in prod for one medium sized instance with ~70 multitenant sites. So far so good with its usage. |
Sure! I'll put together something based on this PR and link it here when ready. |
Introduces `useAppTheme` in `AppProvider` to load/inject `<link>` elements for the core theme CSS and any theme variant CSS into the HTML document. Exposes the app theme state and a way to mutate it to consumers via `AppContext`.
adamstankiewicz
force-pushed
the
ags/inject-theme-css
branch
from
f790b9d to
697e43b
Compare
@xitij2000 I ended up putting together a possible solution to prefer config from the MFE config API; otherwise, fallback to environment variables. I just pushed up the latest changes to this PR. The runtime config vs. environment variable config approaches are described in a new |
* test: add testing to useParagonThemeCore * test: add test to useThemeVariants hook * fix: Paragon definition and remove onload mock * test: change test message to be clear
|
I was testing system preference configuration and it works when default and brandOverride are defined but when I use only default values like: can not change the theme properly: scrnli_7_27_2023_4-51-10.PM.webm |
| } else { | ||
| const updatedStylesheetRel = generateStylesheetRelAttr(themeVariant); | ||
| existingThemeVariantLink.rel = updatedStylesheetRel; | ||
| existingThemeVariantBrandLink.rel = updatedStylesheetRel; |
There was a problem hiding this comment.
Need an extra conditional when brand link is not defined
| existingThemeVariantBrandLink.rel = updatedStylesheetRel; | |
| if (existingThemeVariantBrandLink) { existingThemeVariantBrandLink.rel = updatedStylesheetRel; } |
| return; | ||
| } | ||
| const getParagonThemeCoreLink = () => document.head.querySelector('link[data-paragon-theme-core="true"'); | ||
| const existingCoreThemeLink = document.head.querySelector(`link[href='${themeCore.urls.default}']`); |
There was a problem hiding this comment.
I've been testing this while migrating an MFE to using runtime theming, and I think these queries are causing issues in some cases.
The frontend-build PR is adding these links a <link rel=preload which is also picked up by such queries, meaning that it doesn't attempt to add the actual links.
I think this can be fixed as follows:
| const existingCoreThemeLink = document.head.querySelector(`link[href='${themeCore.urls.default}']`); | |
| const existingCoreThemeLink = document.head.querySelector(`link[href='${themeCore.urls.default}'][rel=stylesheet]`); |
There was a problem hiding this comment.
I think a similar change will need to be applied to other queries as well.
This PR purpose is to test/demo parago design tokens simliar to this one for the profile openedx/frontend-app-profile/pull/764 it override the following depns as seen in package.json - paragon alpha - openedx/frontend-build/pull/365 - openedx/frontend-platform/pull/440 - openedx/frontend-component-header/pull/351 - openedx/frontend-component-footer/pull/303 Conclousion so far: - There is an extra library that learning depends on which needs to support paragon; `frontend-lib-learning-assistant` and also `frontend-lib-special-exams` - It would be great to have a gudie or a document to look at, while doing the conversion that would **map variable from who it was used/named before to the name in design tokens** - I was stuck in the end with compliation error, that wepack couldn't find `Modal` exported from paragon.
|
Hey @adamstankiewicz , What is the current status of this PR, is it ready to review and merge? |
Co-authored-by: monteri <lansevermore>
…atform into ags/inject-theme-css
| return function cleanup() { | ||
| unsubscribe(subscriptionToken); | ||
| }; |
There was a problem hiding this comment.
Is possible to change this to an arrow function?
| return function cleanup() { | |
| unsubscribe(subscriptionToken); | |
| }; | |
| return () => unsubscribe(subscriptionToken); |
|
@adamstankiewicz Is there any way we can help get this closer to merging? We're currently using this in conjunction with patched versions of MFEs and it's working pretty well. |
|
This was discussed today in an FWG meeting, It sounds like this will be taken forward as part of a Design Token Axim Funded Contribution. |
|
Closed in favor of openedx/frontend-build#546, which picked up where this PR left off :) |
Description:
Implementation POC/prototype for ADR on loading a common, external stylesheet for MFEs.
useParagonThemeinAppProviderto load/inject<link>elements for the core theme CSS and any theme variant CSS into the HTML document.AppContext.localStorageso it persists/loads across sessions and page refreshes.localStorage, checks if the user's System Preferences has a preference for dark mode via a@mediaquery forprefers-color-scheme: dark. If dark color scheme is preferred, the default configured dark theme variant is made visible, if one exists.prefers-color-scheme: darkquery changes (i.e., user updates their System Preferences), loads the appropriate default theme variant corresponding to the system preference in real time.See theming.md for documentation and more details.
Merge checklist:
frontend-platform. This can be done by runningnpm startand opening http://localhost:8080.module.config.jsfile infrontend-build.fix,feat) and is appropriate for your code change. Consider whether your code is a breaking change, and modify your commit accordingly.Post merge: