fix: update env.config.jsx according to tutor-mfe#240 PR #109
Conversation
tutorindigo/plugin.py
Outdated
| import typing as t | ||
|
|
||
| import importlib_resources | ||
| from glob import glob |
There was a problem hiding this comment.
One of these two imports is unnecessary.
| @@ -57,7 +57,7 @@ | |||
| .xmodule_display.xmodule_HtmlBlock ul, | |||
| .xmodule_display.xmodule_StaticTabBlock ol, | |||
| .xmodule_display.xmodule_StaticTabBlock ul { | |||
| color: $text-color-d; | |||
| color: $text-color-d !important; | |||
There was a problem hiding this comment.
AFAIK !important css tags are bad practice. Is there a legitimate reason to use them here? Is there any way to avoid them?
There was a problem hiding this comment.
The CSS changes will be removed in this PR. We've done in #111
| @@ -0,0 +1,4 @@ | |||
|
|
|||
|
|
|||
| const Footer = await import(/* webpackIgnore: true */`@edly-io/indigo-frontend-component-footer`).catch(() => null); // Fallback to null if not available | |||
There was a problem hiding this comment.
| const Footer = await import(/* webpackIgnore: true */`@edly-io/indigo-frontend-component-footer`).catch(() => null); // Fallback to null if not available | |
| const { default: IndigoFooter } = await import('@edly-io/indigo-frontend-component-footer'); |
First: instead of having to use Footer.default below, it's less confusing to just rename default to the component we want. (I suggest IndigoFooter to avoid confusion.)
Second: as far as I can tell, webpackIgnore will make it so webpack never imports the footer component, so we can't use that. And I'm not convinced a catch would work as intended, here. This is already in a try/catch block, which is the only way to get Webpack to actually do an optional import as of v5.90.2.
Now, this does indeed break the build in frontend-app-ora-grading: webpack is not generating code that treats the import as optional. I tracked this down to the fact this MFE is not configuring browserslist properly in package.json (see PR that fixes it). We'll fix the MFE, but I have a question in the meantime: any particular reason why the Indigo footer is not being npm-installed for frontend-app-ora-grading?
There was a problem hiding this comment.
any particular reason why the Indigo footer is not being npm-installed for frontend-app-ora-grading?
Adding IndigoFooter to ORA requires styles from indigo-brand-openedx package (otherwise indigo-footer doesn't look appealing). And if we apply indigo-brand-openedx package, then dark-theme automatically applies to body and it shows inconsistent, unreadable page of ora-grading MFE. We have planned to work on ora-grading styling after sumac. We'll include at that time.
There was a problem hiding this comment.
Got it, thanks!
I'm hoping to get the ORA grading MFE fixed today (including in the Sumac branch).
There was a problem hiding this comment.
A couple of suggestions, but otherwise looks good to me!
FYI, both ora-grading and learner-dashboard have been fixed so that optional imports work (openedx/frontend-app-ora-grading#380, openedx/frontend-app-learner-dashboard#524).
|
Thanks everyone for your continuous support & help. I've tested on my system.
Tests: (All tests are successful)
We just need confirmation from one more team member regarding testing, and then we’ll be ready to proceed with the merge. |
hinakhadim
force-pushed
the
fix/update-envConfigJsx
branch
from
b1474e1 to
df5abfb
Compare
|
I tested the changes with the updated branches of tutor-indigo and tutor-mfe and they seem to be working as expected! 🚀 |
* fix: update env.config.jsx according to tutor-mfe#240 PR * chore: install tutor-mfe main branch
Add env-patches to add code in
env.config.jsxand updated plugin.py for updating footer_slot.Update required for overhangio/tutor-mfe#240