Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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: adds support for loading external theme CSS for MFEs #440
feat: adds support for loading external theme CSS for MFEs #440
Changes from 10 commits
3cde02d
f41dcb2
2b19931
347957b
ef52e41
2849bfd
a298bf5
697e43b
fbfd722
a2b3f99
b477c8d
916c1b3
4b2038c
26fbfac
b259760
155bf03
e5ee81e
813169d
a714d49
1e13ac3
e2b0df9
ea99e3c
00fd9c2
0da2cfa
f24c336
84b34ee
f9aa947
fb70be9
e0768ff
4c5f358
c957eb5
2c386ac
340e259
9b5bfa5
59401f5
46cb39a
ebbe03a
494ad62
74f9f8f
e3e5fe5
902e8f4
0c73b5b
b380c18
67a0a1d
c30ae3f
e2a115e
33a8377
45c727e
f6d633c
d65acff
efdf60c
e96de6b
8f39517
cf10278
8f0edb4
084a4ed
b5f1588
2b2772e
3345dc0
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Nit: To me this paragraph sort of implies that the MFE config API in edx-platform is one example of how this can be done, rather than the sole way frontend-platform can get this info. (I mean, I guess someone could configure the frontend to point at some other service, but none exists!)
A link to the MFE config API doc here would probably be helpful.
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.
Agreed. Re-worded, and will push up a fix shortly.
@davidjoy Is there a suitable link to include here? I'm not aware of one. I know I started to create this (currently 2U-internal) document describing the different configuration mechanisms because I couldn't find any other documentation about the MFE runtime config API.
That being said, yes, a link to some more detailed, public docs here would be helpful :)
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 think this is a good starting point: https://github.com/openedx/edx-platform/blob/master/lms/djangoapps/mfe_config_api/docs/decisions/0001-mfe-config-api.rst
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've updated the language to include this above suggested link.
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.
Would it make sense to support variable substitutions here? i.e. the URL could be:
https://cdn.jsdelivr.net/npm/@edx/paragon@$paragonVersion/dist/paragon.css
This way with each upgrade you don't need to update this value here in a completely different place.
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.
The last version of Paragon Alpha creates a folder instead of a file (core, and theme variant)
I was wondering if this implementation should take that into consideration or should be resolved by the jsdelivr capacity to load more than one file per request https://www.jsdelivr.com/documentation#id-combine-multiple-files?
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.
@xitij2000 Can you expand a bit more on how you think the variable substitution would work here? Where would
$paragonVersion
come from?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.
@dcoa That the output you're showing is from the
npm run build-tokens
command. We have another commandnode build-scss.js
(or vianpm run build
) that compiles all of Paragon's SCSS and these generated CSS files into the expected output files in thedist
folder:core.css
core.min.css
core.css.map
light.css
light.min.css
light.css.map
These compiled CSS files during this build step are the ones intended to hosted on a CDN, where this
@edx/frontend-platform
solution would pull from.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.
The idea with variable substitution is that if you upgrade the version of paragon in the MFE it should automatically inject its version of paragon into this URL instead of requiring you to update the MFE configuration is site-configuration, or in settings.
The version of Paragon to be filled in here is known at build time since it's in package.json so it can just be a static variable auto-discovered from the package.json file.
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.
[inform] I pushed up a commit using the above linked version of
@edx/frontend-build
to demonstrate the usage of thePARAGON_VERSION
global variable.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.
Assuming I am not incorrect, this would lead to style shift, right? because the flow as I understnad would as
Am I right, with the timeline of events above?, either cases I think the use PARAGON_VERSION is valuable, but we probably shall ask if sudden style change is okay and if not how to resolve it...
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.
The problem I am referring to is exactly this https://css-tricks.com/content-jumping-avoid/
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.
Reading into the thread, I think my comment above, is related even if we don't define
PARAGON_VERSION
. i.e even if just definePARAGON_THEME_URLS
, wouldn't the MFE at first load the deafult theme that was used/set when the MFE is built, before it get a chance to update it using via the dynamic config API?.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.
@ghassanmas I believe the intent here is that the MFE would no longer import any of Paragon's theme SCSS, in favor of having
@edx/frontend-platform
inject the compiled theme CSS viaAppProvider
at runtime, or otherwise fallback to the CSS included in the locally installed Paragon verison.In this case, the MFE isn't shown until the externally loaded CSS files are injected/loaded by the browser, similar to how the MFE isn't rendered until the request to fetch the dynamic MFE config API is resolved.
See the below graphic for the flow in this proposed implementation for injecting/loading the Paragon theme URLs (I'll plan on adding a similar graphic to the ADR in this PR as well):