openedx / wg-build-test-release

Open edX Build / Test / Release Working Group
25 stars 15 forks source link

Support for the Learning MFE #81

Closed BbrSofiane closed 2 years ago

BbrSofiane commented 3 years ago

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.

pdpinch commented 3 years ago

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

pdpinch commented 3 years ago

Our findings so far:

arbrandes commented 3 years ago

@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 commented 3 years ago

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

arbrandes commented 3 years ago

@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 commented 3 years ago

@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 commented 3 years ago

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 commented 3 years ago

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

pdpinch commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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

pdpinch commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

@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 commented 3 years ago

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 commented 3 years ago

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

davidjoy commented 3 years ago

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 commented 3 years ago

@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 commented 3 years ago

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 commented 3 years ago

@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 commented 3 years ago

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 commented 3 years ago

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

davidjoy commented 3 years ago

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 commented 3 years ago

@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:

UPD:

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 commented 3 years ago

@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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

@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 commented 3 years ago

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 commented 3 years ago

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

davidjoy commented 3 years ago

@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 commented 3 years ago

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

cmltaWt0 commented 3 years ago

@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 commented 3 years ago

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

ghassanmas commented 3 years ago

@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 commented 3 years ago

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 commented 1 year ago

hi

rahulkg666 commented 1 year ago

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

kdmccormick commented 1 year ago

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