Skip to content
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

Support for the Learning MFE #81

Closed
BbrSofiane opened this issue Jun 7, 2021 · 41 comments
Closed

Support for the Learning MFE #81

BbrSofiane opened this issue Jun 7, 2021 · 41 comments
Assignees
Milestone

Comments

@BbrSofiane
Copy link
Member

BbrSofiane commented Jun 7, 2021

Description

Coordinate, and if necessary execute, the tasks necessary to get the Learning MFE up to speed for Maple.

Acceptance Criteria

The Learning MFE should:

  1. Work as intended with the rest of the Maple codebase.
  2. Pass a minimum standard of documentation: at the very least, the READMEs should have descriptive text that includes use cases, screenshots, and particularly, documentation on that MFE's environment variables.
  3. Be internationalized (i18n), localizable (l10n), and pass minimum accessibility (a11y) standards.
  4. Be deployable via Tutor as a plugin.
  5. It has a header and footer that are customizable via npm aliases: https://edx.readthedocs.io/projects/edx-developer-docs/en/latest/developers_guide/micro_frontends_in_open_edx.html#overriding-brand-specific-elements
  6. It is compatible with the @edx/brand system: https://open-edx-proposals.readthedocs.io/en/latest/oep-0048-brand-customization.html
  7. It uses CSS utility classes in a branding-friendly way. (i.e., don't use things like 'bg-white')

Wishlist

The following is a collection of what the community has expressed interest in. Any single item may be "upgraded" to the Acceptance Criteria above, pending discussion.

  • The MFE should support path-based routing (added by @pdpinch)
@BbrSofiane BbrSofiane added this to the Maple.1 milestone Jun 7, 2021
@pdpinch
Copy link

pdpinch commented Jul 19, 2021

Path-based routing doesn't appear to be implemented in the learning MFE. Is that a requirement for Maple?

@pdpinch
Copy link

pdpinch commented Jul 20, 2021

Our findings so far:

  • If you use domain-based routing, you will need to add the MFE domains to CORS_ORIGIN_WHITELIST and, possibly, CSRF_TRUSTED_ORIGINS
  • MFE Authentication requires the full complement of JWT settings. Since the individual settings aren't documented, we're not sure what these are for and how they interact with the MFE.

@arbrandes
Copy link
Contributor

@cmltaWt0, since you did such a great job on the ecommerce MFEs, would you be interested in investigating what needs to be done to get the learning MFE up and running on current master?

At this point we're mostly interested in how easy/difficult it would be just to get it working. We're not after a PR to edx/configuration, for instance. (The end goal would actually be a PR to Tutor, when/if we get there.)

@nedbat
Copy link
Contributor

nedbat commented Jul 20, 2021

Can we have some more specifics for "reasonably themeable"? Are the current Lilac MFE's reasonably themeable?

@arbrandes
Copy link
Contributor

@nedbat, good question. IIRC, this line comes from when we were discussing what to do about the MFEs we would support for Lilac - before we found out how hard theming would be. As far as I'm concerned, we can ditch that line. (Possibly in favor of a dedicated issue to keep track of theming developments. #36 would be a good start, for instance.)

@cmltaWt0
Copy link

@cmltaWt0, since you did such a great job on the ecommerce MFEs, would you be interested in investigating what needs to be done to get the learning MFE up and running on current master?

At this point we're mostly interested in how easy/difficult it would be just to get it working. We're not after a PR to edx/configuration, for instance. (The end goal would actually be a PR to Tutor, when/if we get there.)

Yeap. I think it's reasonable (not such reasonable as reasonably themeable but anyway :) )

I'll participate in this investigation.

@pdpinch
Copy link

pdpinch commented Jul 26, 2021

There are four .env settings in frontend-app-learning for logos. What are they for? Is it possible to annotate these similar to other settings?

from https://github.com/edx/frontend-app-learning/blob/a003059c8f70fc8371d0ef22a83b0bad2ca23f4f/.env#L15-L18

LOGO_URL=''
LOGO_TRADEMARK_URL=''
LOGO_WHITE_URL=''
FAVICON_URL=''

For example, is the LOGO_TRADEMARK_URL really supposed to be configurable by the operator?

@jmyatt
Copy link

jmyatt commented Jul 26, 2021

@pdpinch does this now-merged OSPR address your question on path-based routing? https://openedx.atlassian.net/browse/OSPR-5847

@pdpinch
Copy link

pdpinch commented Aug 3, 2021

Thanks for the link @jmyatt. I think that commit was necessary for other MFEs to work, but frontend-app-learning is still missing the basic support.

@mikix
Copy link

mikix commented Aug 5, 2021

Those four variables are mentioned in the general MFE developer docs:
https://edx.readthedocs.io/projects/edx-developer-docs/en/latest/developers_guide/micro_frontends_in_open_edx.html#branding

@pdpinch
Copy link

pdpinch commented Aug 11, 2021

FYI, we're starting work on making frontend-app-learning support path-based routing like the other MFEs.

@pdpinch
Copy link

pdpinch commented Aug 11, 2021

This is probably out-of-scope, but we used to use the old theming system to override the items in the user menu. Is there a best practice way to do that with MFEs?

image

@mikix
Copy link

mikix commented Aug 24, 2021

This is probably out-of-scope, but we used to use the old theming system to override the items in the user menu. Is there a best practice way to do that with MFEs?

The way I know to theme frontends is to extend the default theme and use your new custom theme. This repo's readme has instructions: https://github.com/edx/brand-openedx

Is that what you meant?

@regisb
Copy link
Contributor

regisb commented Sep 9, 2021

Looks like the learning MFE is working in Tutor :)

tutor learning

I am currently working on getting Tutor to run with the Open edX master branches; the result will be available in the "edge" branches of the tutor main repo and the mfe plugin.

That's it! I just wanted to share this small achievement :-)

@pdpinch
Copy link

pdpinch commented Sep 14, 2021

@regisb how are you handling the routing in Tutor? (I should look and see for myself, but I thought I'd ask if it's a quick question)

@regisb
Copy link
Contributor

regisb commented Sep 14, 2021

how are you handling the routing in Tutor?

Traffic first goes through Caddy (for SSL termination), then Nginx (for proxying), then another Caddy server (in the MFE container). The MFE container holds all the static assets for all microfrontends. The Caddy server in this MFE container serves static assets from multiple directory locations. Here is the Caddy configuration file: https://github.com/overhangio/tutor-mfe/blob/master/tutormfe/templates/mfe/build/mfe/Caddyfile The following values are added to the LMS settings: https://github.com/overhangio/tutor-mfe/blob/master/tutormfe/patches/openedx-lms-production-settings

Does that answer your question?

@BbrSofiane
Copy link
Member Author

https://discuss.overhang.io/t/running-open-edx-on-master-with-tutor-edge/1957/4

@davidjoy
Copy link

FYI, the learning MFE has a hard-coded header and doesn't use the npm aliases mechanism described here: https://edx.readthedocs.io/projects/edx-developer-docs/en/latest/developers_guide/micro_frontends_in_open_edx.html#overriding-brand-specific-elements

This means that the header will be brandable (change styling), but not override-able (change functionality) today.

@pdpinch
Copy link

pdpinch commented Sep 27, 2021

@davidjoy do you know what would be involved in making the header (and footer) override-able? Is that something that could be fixed in an OSPR?

@davidjoy
Copy link

davidjoy commented Sep 27, 2021

Yes... I think we could. It might be a smidge complicated.

I think the right thing to do is to move a basic, generic version of the header in the learning MFE into the frontend-component-header repository and export it from there as something like LearningHeader as a peer of the Header export. Then give the MFE a dependency on that and pull the code from it instead. Before we could merge the PR in the learning MFE, we'd also need to copy that code into frontend-component-header-edx so that edx.org's build process doesn't explode, since the site runs on master. Then the two headers could diverge from there as needed.

In the future, we could also add other headers (like StudioHeader) into frontend-component-header, which could then be used in MFEs like frontend-app-course-authoring.

@regisb
Copy link
Contributor

regisb commented Sep 27, 2021

@davidjoy I did not dig deep enough into the MFE source code to fully understand your explanations above; nonetheless, the process seems rather complex for a task that is a frequent requirement of most Open edX operators. Is there any way to improve and document the theming of MFEs to tell our users a better story?

@davidjoy
Copy link

This task would be a one-time refactor of the code in the learning MFE that would enable operators to make their own customized versions of the header. The existing mechanism for doing that is to use an npm alias to point @edx/frontend-component-header to my-frontend-component-header - the problem is that the learning MFE just doesn't get its header from that package today; it's hardcoded in the app.

The process of using the npm alias is described here: https://edx.readthedocs.io/projects/edx-developer-docs/en/latest/developers_guide/micro_frontends_in_open_edx.html#overriding-brand-specific-elements

@cmltaWt0
Copy link

cmltaWt0 commented Oct 5, 2021

As far as I understand the same architecture as for frontend-app-ecommerce should be applied.
image

@davidjoy
Copy link

davidjoy commented Oct 5, 2021

Yup - it should be very similar. I think line 1 of index.jsx ends up looking like:

import LearningHeader, { messages as headerMessages } from '@edx/frontend-component-header';

... for instance, since we'll need to move the existing header into that repo and then use it.

@cmltaWt0
Copy link

cmltaWt0 commented Oct 6, 2021

@regisb
I'm doing some testing for learning MFE on local tutor installation (non production mode).

And I've encountered missing waffle flag in hooks for lms.
Added in PR.

Next we could discuss the following flags to add:

course_home.course_home_mfe (REQUIRED FOR FOLLOWING FLAGS)
course_home.course_home_mfe_dates_tab (OPTIONAL)
course_home.course_home_mfe_outline_tab(OPTIONAL)
course_home.course_home_mfe_progress_tab(OPTIONAL)

Also I can't get the fully working learning app - it still stuck in Loading state (deployed from nightly branches from scratch).
I'll play around to apply my previous fixes or we can meet to resolve it more quickly because I don't understand how it's working for you.

image

Next steps:

  • find out the fix for infinite loading on local tutor installation
  • test production mode
  • test forked learning app repo to apply shared Header and Footer as a npm packages
  • create a PR to learning app repo

UPD:

  • I've applied straightforward fix to the edx-platform

image

- applied appropriate config change to the tutor-mfe

image

As a result the learning app is perfectly working:
image

@regisb we definitely must compare our installations to get what I've missed (because it simply doesn't work by default).

UPD2: I've managed to make tutor works with latest MFEs. Couple PRs were and will be opened to fix some issues blocked us from running it smoothly on the tutor nightly + edx-platform master.

@jmyatt
Copy link

jmyatt commented Oct 6, 2021

@cmltaWt0 about the flags you mentioned, only course_home.course_home_mfe_progress_tab should still be relevant.

Removal of course_home.course_home_mfe_dates_tab and course_home.course_home_mfe_outline_tab was done in this commit

Removal of course_home.course_home_mfe was done in this commit

@cmltaWt0
Copy link

cmltaWt0 commented Oct 6, 2021

Thanks @jmyatt !
I've missed that nighty is still on lilac.2 by default (or at least it is for me).

@regisb It's my fault with flag in tutor-mfe's PR.
New courseware was enabled by default in https://github.com/edx/edx-platform/pull/27757

I have to revert the PR (it makes no effect on the logic).

@asadiqbal08
Copy link

Yes... I think we could. It might be a smidge complicated.

I think the right thing to do is to move a basic, generic version of the header in the learning MFE into the frontend-component-header repository and export it from there as something like LearningHeader as a peer of the Header export. Then give the MFE a dependency on that and pull the code from it instead. Before we could merge the PR in the learning MFE, we'd also need to copy that code into frontend-component-header-edx so that edx.org's build process doesn't explode, since the site runs on master. Then the two headers could diverge from there as needed.

In the future, we could also add other headers (like StudioHeader) into frontend-component-header, which could then be used in MFEs like frontend-app-course-authoring.

@davidjoy related to the suggestion above, Can you please take a look over the approach here ?

@cmltaWt0
Copy link

cmltaWt0 commented Oct 7, 2021

Side question - do we have a list of changes in UI/UX compared to legacy Header so we can know the desired Header's functionality.

What I can see right know - is missing Discover New link in the Learning MFE Header.

Screenshot 2021-10-07 at 16 00 22

Screenshot 2021-10-07 at 16 00 36

@cmltaWt0
Copy link

cmltaWt0 commented Oct 8, 2021

@davidjoy Could you support me in the comment above by pointing a person to ask for the list of features or it's simpler to create an issue in the learning repo?

@asadiqbal08
Copy link

Yes... I think we could. It might be a smidge complicated.
I think the right thing to do is to move a basic, generic version of the header in the learning MFE into the frontend-component-header repository and export it from there as something like LearningHeader as a peer of the Header export. Then give the MFE a dependency on that and pull the code from it instead. Before we could merge the PR in the learning MFE, we'd also need to copy that code into frontend-component-header-edx so that edx.org's build process doesn't explode, since the site runs on master. Then the two headers could diverge from there as needed.
In the future, we could also add other headers (like StudioHeader) into frontend-component-header, which could then be used in MFEs like frontend-app-course-authoring.

@davidjoy related to the suggestion above, Can you please take a look over the approach here ?

@davidjoy

@davidjoy
Copy link

Thanks for this - I left a few comments over there which I think can help reduce the scope.

@davidjoy
Copy link

@davidjoy Could you support me in the comment above by pointing a person to ask for the list of features or it's simpler to create an issue in the learning repo?

I believe the "Discover New" link was removed intentionally when we created the header in the learning MFE.

Are you looking for a list of features that are in the Learning MFE's header today? Otherwise I don't think it's changed too much from the legacy one.

@BbrSofiane
Copy link
Member Author

Frontend WG is also tracking this issue: openedx/wg-frontend#24

@cmltaWt0
Copy link

@davidjoy Could you support me in the comment above by pointing a person to ask for the list of features or it's simpler to create an issue in the learning repo?

I believe the "Discover New" link was removed intentionally when we created the header in the learning MFE.

Are you looking for a list of features that are in the Learning MFE's header today? Otherwise I don't think it's changed too much from the legacy one.

If it's just one intentional change - it's fine. I'm just wondering about any other changes could be missed unintentionally.

@davidjoy
Copy link

My gut/memory says that's the one intentional change.

@ghassanmas
Copy link
Member

ghassanmas commented Oct 26, 2021

@davidjoy following your comment/suggestion here, I have been discussing that with @pdpinch and @asadiqbal08,

You are suggesting to export different versions of the header inside the header-componenet, so for an app that is using the header, it would feel that it's a collection of headers types..etc.

Regarding how to implement that, I would assume that we should not create different/separate components for each use case, e.g. learning, studio, standard..etc. And should rather make sure that all these versions are inheriting the standard one. Why? So that if we want to change something in the future, we shouldn't look for every version of the component and change it..

Do you agree with my assumption or is it too early to decide how the architecture of the component should be

@davidjoy
Copy link

My expectation is that the frontend-component-header repository would export a collection of headers specific to different domains (marketing, LMS, courseware, studio, ecommerce, etc.) and different MFEs would use the one appropriate to their domain.

The components we export, like LearningHeader, StudioHeader, or EcommerceHeader could share sub-components as appropriate. One example would be to unify on a single implementation of a UserMenu, or the Logo on the left, and to reuse the code that makes the headers mobile responsive, etc. The exact details of this unification can be deferred, in my opinion, as our goal right now is just to get the learning MFE supported in an Open edX release.

Future: I expect that over time we can add configuration props to these headers to make them a bit more flexible, reducing the need for many operators to fork the frontend-component-header repository. The micro-frontend configuration 2.0 epic in the FWG (https://github.com/openedx/frontend-wg/issues?q=is%3Aopen+is%3Aissue+milestone%3A%22MFE+Config+2.0%22) is probably a pre-requisite for this, as it will allow us more flexibility and expressiveness when configuring MFEs. Specifically, it would allow us to pass an Object of properties to be passed to the header from environment-specific configuration.

@rahulkg666
Copy link

hi

@rahulkg666
Copy link

Hi I need to know how can I add copy protection in mfe learning app

@kdmccormick
Copy link
Member

Hi @rahulkg666 . This is not the right place to ask for help. Please use the forums instead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

No branches or pull requests