Closed arbrandes closed 2 years ago
@regisb, can we count on you for Tutor support for this, as well as for #28 and #27?
(It doesn't mean you're the only assignee, here, just that you'll help - or at least kickstart - the Tutor bits. :)
Yes, the required MFEs will be delivered for Tutor in v12.
@arbrandes -- given the MFE is currently not internationalized, are we going to put this MFE aside for the more critical ecommerce MFEs?
cc @kdmccormick
@sarina, apologies for the protracted delay, but as we've already discussed in the meeting, yup, we're dropping this one.
Good Morning :) I am going to be reviewing this. I plan to review it today.
@jnlapierre is the edX contact for this work. Thanks Justin!
Thanks @nedbat, @jnlapierre Please let me know if you require any help in the review you're going to perform.
Hi @jnlapierre did you have the chance to review the Gradebook MFE?
@davidjoy @jnlapierre hi, do you have any updates related to the Gradebook MFE?
@BbrSofiane I'm in the process of testing the MFE in Tutor. I've found an issue related to the MFE running in a subdirectory instead of a subdomain. The line here extracts the course_id string from the window location and assumes all the implementations of the MFE use a subdomain. If the MFE lives in a subdirectory (for instance https://mydomain.com/gradebook/
@davidjoy @jnlapierre a couple of questions here:
Thank you in advance
That link describes how to do the header/footer overrides, yes. It appears that gradebook is only using the frontend-component-footer
repo, but not the header. That means it won't work for the header.
To test out branding (changing styles), see directions here: https://github.com/edx/brand-openedx
I can't speak to the internationalization state. I'm still trying to get some time from someone on @jnlapierre's team to help us.
@davidjoy @jnlapierre I added a PR to fix the problem of the courseId calculation here. Could you please help me with a review? Thanks!
@BbrSofiane I think this is not ready to be marked as DONE. I'll be working on checking the branding, header and footer overrides and i18n
@jfavellar90 yes, I meant to close the other issue.
Comments related to this task:
mfe-dockerfile-post-npm-install
offered by Tutor to install a fork of the edx/brand-openedx repo (https://github.com/eduNEXT/brand-openedx/) with a couple of style changes. It worked as expected. The line to install the custom branding repo was the following]:RUN npm install --no-audit --no-fund @edx/brand@git+https://github.com/eduNEXT/brand-openedx.git --save
I had issues running the footer override. I just forked the base repo and applied a couple of changes on the last tagged version (v10.1.6). Then tried to install the footer with npm aliases through the same mfe-dockerfile-post-npm-install
patch. When building the grade book app with the override I get the error HERE
Doing a little research I found this post from where I took a couple of ideas to fix the issue. Specifically, I tried this and this, obtaining the same failure. @davidjoy could you please take a look? I suspect this is something easy to fix.
This MFE does not support header override. We need to decide if this is a must-have for the final release.
The PR related to the course extraction fix is still waiting for a review. I already tagged again the team in charge of the repo.
Related to i18n, @regisb has been working on creating the project in Transifex for this MFE and created a PR with the first version of the translations pulled from Transifex.
I'm pretty sure the import shouldn't be prefixed with an underscore _
:
@import "~@edx/frontend-component-footer/dist/_footer";
It's a fragment, so SASS should find it without that.
I think it should be:
@import "~@edx/frontend-component-footer/dist/footer";
FYI, regarding the header, @asadiqbal08 has the follow PRs in-flight for the Learning MFE:
https://github.com/edx/frontend-app-learning/pull/715 https://github.com/edx/frontend-component-header/pull/133 https://github.com/edx/frontend-component-header-edx/pull/196
@davidjoy According to my testing, the problem with the footer override is that dist
folder is not included in the GitHub repos. I had to manually stop ignoring the dist
folder in my footer component fork (https://github.com/eduNEXT/frontend-component-footer/commit/89015dc1fec7da89313b0989181bc7463216c7ba) and install the component from the repo. Once this is done, the footer override works as expected. I'm not pretty sure this is a good practice but it was the only way I got it working. any thoughts related to this approach?
Once this is done, the footer override works as expected. I'm not pretty sure this is a good practice but it was the only way I got it working. any thoughts related to this approach?
@davidjoy any thoughts on this?
@jfavellar90 is this the last hurdle to for the gradebook MFE to be ready?
@BbrSofiane we need to get merged this PR: https://github.com/edx/frontend-app-gradebook/pull/213 in order to make the MFE work properly in tutor, @davidjoy is there any way to accelerate this process? Once merged it must be backported to Gradebook maple release
I have another PR in https://github.com/edx/frontend-app-gradebook/pull/220 related to the frontend-component-header
implementation, but this is not a must-have for the Maple release (this PR can take a bit longer to get merged).
On the other hand, we need to backport the language work that Regis already got merged in master. I was wondering if we can reset maple release branch (open-release/maple.beta1
) to the current master.
Summarizing, the minimum required for the release is:
I remain super attentive to any feedback.
edx/frontend-app-gradebook#218 - backport to maple
@BbrSofiane backport course id issue https://github.com/edx/frontend-app-gradebook/pull/221
merge edx/frontend-app-gradebook#222
Coordinate, and if necessary execute, the tasks necessary to get the Gradebook MFE up to speed for Lilac: