-
Notifications
You must be signed in to change notification settings - Fork 94
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: configure style via MFE config api #311
Conversation
Thanks for the pull request, @navinkarkera! Please note that it may take us up to several weeks or months to complete a review and merge your PR. Feel free to add as much of the following information to the ticket as you can:
All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here. Please let us know once your PR is ready for our review and all tests are green. |
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## master #311 +/- ##
==========================================
+ Coverage 81.25% 84.21% +2.96%
==========================================
Files 4 4
Lines 32 38 +6
Branches 4 8 +4
==========================================
+ Hits 26 32 +6
Misses 6 6
☔ View full report in Codecov by Sentry. |
43efd5a
to
d1e6938
Compare
👍
|
0ae617e
to
9161f00
Compare
@mphilbrick211 This is ready for you. |
Hi @openedx/fed-bom! Would someone be able to review/merge this for us? Thanks! |
Just a quick note: @adamstankiewicz has brought it up that this seems to be partly orthogonal to what openedx/frontend-platform#440 is doing, so it warrants some additional discussion, particularly with regards to the styling mechanism. (The link URL manipulation is not as controversial, I would think.) |
On a closer look, it does appear this PR is getting at both branding customization (i.e., colors, spacing, etc.) and theming (a la comprehensive theming, customizing the functionality). What the above linked PR is doing is about ingesting Paragon's (compiled) CSS via MFE runtime config API whereas this PR is seemingly largely getting at the issue that the component, as implemented, is not flexible/customizable (e.g., to be able to override/extend the links used in the footer, etc.). That said, the branding implications are probably separate from Paragon's design tokens / CSS variables given I'm curious about other alternative approaches you've considered/tried and why they were less advantageous to your current approach. For example, could
// individual MFE
:root {
--openedx-footer-background: url(https://encrypted-tbn0.gstatic.com/images?q=tbn:ANd9GcTgb0Xw3iHTv4pobzu3cSStIU3txTxzC1LHNw&usqp=CAU);
} I think a potential concern here is that it doesn't feel intuitive at first glance for engineers to modify frontend styles via a Python configuration setting. That said, it does help support writing that customization in 1 place and having it propagate to all MFEs whereas an expanded props API and/or using CSS variables may not mitigate that concern. That said, is it a safe assumption that all MFEs throughout Open edX would want these customizations, or would they still need to be defined on a MFE-by-MFE basis (fwiw, this latter concern won't be as much of an issue in a world with Domain MFEs via something like Piral given there should only be a single footer in the Domain MFE at any given time, rather than each individual MFE defining its own I feel it'd be good to write down the context for these changes, their implications, and any alternatives considered as an Architectural Decision Record (ADR) in this repository as well. |
@arbrandes @adamstankiewicz Thanks for your inputs.
@adamstankiewicz Yes, in simple terms, this PR allows adding/removing Paragon classes or raw css styling to some parts of footer, whereas the linked PR if I am not wrong, allows modifying those Paragon classes by overwriting them with its own version of compiled CSS.
So one of our clients wants to be able to host multiple microsites/tenants in a single OpenEdx instance and for each tenant/university they want to customize the footer. We managed to setup multsites using eduNexts eox-tenant which supports overriding django settings per tenant. Since MFE_CONFIG_API allows modifying MFE settings during runtime, we chose it over something like Branding API to add these configurations.
Agreed.
We can still configure it separately for each MFE as
I can surely do that. |
9161f00
to
67e3120
Compare
67e3120
to
ff9811d
Compare
docs: update readme with MFE api customization options test: update snapshot with context feat: add option to configure link container classnames
ff9811d
to
fe9e9c4
Compare
@navinkarkera any updates on the ADR suggested by Adam |
@abdullahwaheed Hi, @navinkarkera is on vacation until approximately end of August. We are discussing internally what we can do to compensate for that, and probably will take it over and write an ADR, however I can't confirm right now. |
This is an interesting approach and could be useful to make the platform more customizable in the MFE environment. |
@@ -51,22 +51,58 @@ This library has the following exports: | |||
language from its dropdown. | |||
* supportedLanguages: An array of objects representing available languages. See example below for object shape. | |||
|
|||
Customization via MFE 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.
I think this description should mention all the possible ways to modify the configuration, I mean, could include the JavaScript File https://github.com/openedx/frontend-platform/blob/master/docs/decisions/0007-javascript-file-configuration.rst
href={element.url} | ||
target={element.target || 'blank'} | ||
> | ||
{element.text} |
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.
This wouldn't get internationalized.
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.
AFAIK, the links in the branding API and translated, so if this PR is switched to using that API, the API should return appropriate results.
However, I don't think it's possible to do the same with the MFE config API. For that you'd need the API to return the translation context for each link and then provide a custom translation mapping. I think it's easier to handle this is edx-platform.
@abdullahwaheed FYI, I'm working on an ADR. The approximate timeline for it is end of this week, beginning of the next one. |
@abdullahwaheed The ADR is ready for review - #326. |
Hi @abdullahwaheed! Just checking in to see if you are able to review/merge this? |
Closing this PR as the ADR was not accepted. |
@navinkarkera Even though your pull request wasn’t merged, please take a moment to answer a two question survey so we can improve your experience in the future. |
Description
This PR adds functionality to customize the footer using MFE config API to some extent. This allows users to customize the footer per site without creating multiple footer components.
Screenshots
Default look with no customization:
With MFE config:
And removal of support languages parameter as shown in diff below:
Results:
Result with no alignment changes:
Test instructions:
make lms-up
.lms/envs/private.py
.npm install
andnpm run start
.lms/envs/private.py
as shown below:example/index.jsx
.