mitodl / mitxonline

BSD 3-Clause "New" or "Revised" License
4 stars 2 forks source link

open edX: standardize user menus #112

Open pdpinch opened 3 years ago

pdpinch commented 3 years ago

As a user, I'd like a consistent user menu across mitxonline and open edX.

While there is some value in a distinct look and feel between mitxonline and open edX, the menu options should have the same text and behave the same.

Here is the current mitxonline:

image

open edX mako template:

image

open edX MFE

image

Acceptance Criteria:

mitxonline changes
asadiqbal08 commented 3 years ago

@pdpinch Related to the discussion on this open-edx thread

I looked over the David's comment and have few findings to share here for your reference.

In learning MFE, at the moment, they are using header from their own code-base referring to this directory but I am unable to properly understand this comment by David.

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

For testing, I checkout the my-own-fork-of-frontend-header-component and used the npm aliases mechanism to override the header but what I did, I imported the header from my-own-fork-of-frontend-header-component something like this in learning MFE code,

import Header from '@edx/frontend-component-header';

instead of

import { Header } from '../course-header';

and having this in my module.config.js in Learning MFE.

module.exports = {
  localModules: [
     { moduleName: '@edx/frontend-component-header', dir: './frontend-component-header-edx', dist: 'src'},
  ]
};

Look on the screenshot below:

screencapture-localhost-2000-course-course-v1-edX-DemoX-Demo-Course-block-v1-edX-DemoX-Demo-Course-type-sequential-block-edx-introduction-block-v1-edX-DemoX-Demo-Course-type-vertical-block-vertical-0270f6de40fc-2021-10-01-16_20_40

In the screenshot below, I have removed the OrderHistory from my-own-fork-of-frontend-header-component

screencapture-localhost-2000-course-course-v1-edX-DemoX-Demo-Course-block-v1-edX-DemoX-Demo-Course-type-sequential-block-edx-introduction-block-v1-edX-DemoX-Demo-Course-type-vertical-block-vertical-0270f6de40fc-2021-10-01-16_40_46

Do you consider, we can use the fork of header-component and use it for customization or still dependent on https://github.com/edx/frontend-app-learning/tree/master/src/course-header

pdpinch commented 3 years ago

Thanks Asad. Are you saying that getting frontend-app-learning to use user frontend-component-header only requires adding a few lines of code?

import Header from '@edx/frontend-component-header';

instead of

import { Header } from '../course-header';

and having this in my module.config.js in Learning MFE.

module.exports = {
  localModules: [
    { moduleName: '@edx/frontend-component-header', dir: './frontend-component-header-edx', dist: 'src'},
 ]
};

Does making that change allow us to use the npm alias method to override frontend-component-header with our own component?

asadiqbal08 commented 3 years ago

@pdpinch Here comes two questions to continue this point.

pdpinch commented 3 years ago

As I mentioned in standup, I think we should address this upstream if we can.

David Joy has said that frontend-app-learning should depend on frontend-component-header like other MFEs do, so if we can do the work I believe edX will merge.

Once frontend-app-learning uses frontend-component-header we should be able to leverage the work @umarmughal824 has already done in https://github.com/edx/frontend-component-header-edx/pull/161 and thereby hide the order history item in the menu by setting the ORDER_HISTORY_URL environment variable to null.

briangrossman commented 3 years ago

@asadiqbal08

I did some testing and found a few more updates that need to be taken care of in this Epic.

From the menu in MFE in MITx Online edX, there are a couple links that need to be updated. (Sample page)

Also, in the menu in the 'legacy' theme, clicking on the username will take you to LMS_BASE_URL/dashboard, but should take you to MARKETING_SITE/dashboard. (Sample page)

Screen Shot 2021-10-26 at 12 00 04 AM

I added this to the mitxonline-theme changes section above (feel free to move it if it's the wrong section)

Let me know if you have any questions.

asadiqbal08 commented 3 years ago

@briangrossman I guess, We can incorporate these changes once the PR merged in mitodl/frontend-component-header-mitol then we just need to update the corresponding .env in our forked repo. @Wassaf-Shahzad fyi.

asadiqbal08 commented 3 years ago

@briangrossman I created a separate story for tracking this change. The thing need to be done once the PR is merged.

asadiqbal08 commented 2 years ago

@pdpinch should I have to keep assign this Epic to myself or are you taking care of it as I am not sure what else is left behind in order complete this ?

@briangrossman FYI

asadiqbal08 commented 2 years ago

@pdpinch

pdpinch commented 2 years ago

@briangrossman is there anything left to do here?

pdpinch commented 11 months ago

@arslanashraf7 this is a super old issue, but I have a couple of questions about it that I think you can help with. To be clear, this isn't urgent in any way.

  1. Do you think all the boxes can be checked off?
  2. How much of this depends on custom code in our forks of the header and footer components?
arslanashraf7 commented 11 months ago

@pdpinch

@arslanashraf7 this is a super old issue, but I have a couple of questions about it that I think you can help with. To be clear, this isn't urgent in any way.

  1. Do you think all the boxes can be checked off?

I'll need to test it or ask someone in the Arbisoft team to test to see how many boxes can we check.

  1. How much of this depends on custom code in our forks of the header and footer components?

Quick Answer:

I think the code changes would only be related to the header and footer since we don't use a fork of the platform or the other MFEs for MITxOnline.

We can see the code changes by looking at the PRs in both the header and footer.

A quick look at the PRs tells us that we didn't make a lot of code changes for the functionality itself. Some PRs were closed and some were only meant to update node versions or dependencies, Readme, etc. The actual code changes are not too large.

If you are looking to get rid of these components, I would say we take a look at the default header and footer of the edX and re-think which of these changes we can achieve as of their latest code without customizing it.

pdpinch commented 11 months ago

Thanks!

I think it makes more sense for me or someone from the MITx team to review the changes. Don’t worry about this for now.

Ferdi commented 11 months ago

As a user, I'd like a consistent user menu across mitxonline and open edX.

Why do they need to be consistent ?