-
Notifications
You must be signed in to change notification settings - Fork 8
feat!: Paragon 23 and External CSS support #111
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
Conversation
This is done in preparation for Paragon 23 support.
68608d2 to
e08c07a
Compare
brian-smith-tcril
left a comment
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.
Super exciting to see this happening! I haven't tested this locally yet, but I did a first pass. Looking forward to seeing where the conversations that stem from my comments go!
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.
@brian-smith-tcril, I addressed the review explicitly in commit a658f31. Now I'll embark on the full-on rename of all variables and filenames as per our previous discussion.
7e22a4f to
a658f31
Compare
|
Ok. @brian-smith-tcril, this is the full diff since your last review (nice, Github now allows linking to the diff of a range of commits in a PR!): Good for a second pass. |
brian-smith-tcril
left a comment
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.
Overall this is looking great!
I left a couple comments, most are small suggestions for changes to docs/inline comments, but there are a couple places where I had some questions.
ecae865 to
0c7e11a
Compare
brian-smith-tcril
left a comment
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.
Looks to me like this is 99% good to merge!
I left a couple tiny comments. Mostly small wording changes to inline comments, one place where adding an extra test would be cool.
brian-smith-tcril
left a comment
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.
Code-wise this LGTM!
I still haven't pulled this down to test locally but I'm still confident to say ![]()
This upgrades Paragon to version 23, which in and of itself is a breaking change to to the introduction of design tokens. In other words, as of this version frontend-base apps now need to support Paragon 23 as well. This also adds a subset of the external CSS feature that was introduced to frontend-build and -platform alongside design tokens. Notably, the following are no longer needed in frontend-base land: * The entire webpackParagonPlugin and the concept of an innate PARAGON_THEME * $paragonVersion and $brandVersion substitutions * Fallback theme URLs In effect, the current port only supports loading `openedx/brand` styles externally. Also, the MFE_CONFIG runtime mechanism is still not finalized for frontend-base, so the corresponding documentation was not added.
Limit react-focus-on's version to `<3.10.0`, as that minor version seems to be a little risky. See: theKashey/react-focus-on#111
15690b7 to
385787e
Compare
Description
This PR does four main things:
While the other commits are reasonably uncontroversial, the meat of it comes from Paragon 23 and design tokens support. Paragon 23 is in and of itself is a breaking change due to the introduction of design tokens. In other words, as of this version frontend-base apps now need to support Paragon 23 as well.
This also adds a subset of the external CSS feature that was introduced to frontend-build and -platform alongside design tokens. Notably, the following are no longer needed in frontend-base land:
In effect, the current port only supports loading
openedx/brandstyles externally.Also, the MFE_CONFIG runtime mechanism is still not finalized for frontend-base, so the corresponding documentation was not added.
References
This is based on the work originally described in the following PRs. They're now closed, but still useful to understand the motivation - including why I'm not including some of the features here:
PARAGONas a global variable frontend-build#365Subsequent PRs actually implemented it:
ParagonWebpackPluginto support design tokens frontend-build#546The migration howto for MFEs is also useful as reference:
https://openedx.atlassian.net/wiki/spaces/BPL/pages/3770744958/Migrating+MFEs+to+Paragon+design+tokens+and+CSS+variables
Testing
Beyond CI, I suggest that this PR undergo only an architecture and code review, and after merge and release of a new version, it will be possible to test the features introduced here more easily with correspondingly upgraded MFEs (PRs listed below).
Related PRs