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

open edX: standardize user menus #112

Open
5 of 12 tasks
pdpinch opened this issue Aug 20, 2021 · 14 comments
Open
5 of 12 tasks

open edX: standardize user menus #112

pdpinch opened this issue Aug 20, 2021 · 14 comments

Comments

@pdpinch
Copy link
Member

pdpinch commented Aug 20, 2021

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
open edX devops changes
  • Redirect LMS_BASE_URL/account/settings in open edX to MARKETING_SITE_URL/account/settings MARKETING_SITE_URL/account-settings (DevOps) (updated)
  • Redirect LMS_BASE_URL/profile/LMS_BASE_URL/u/* in open edX to MARKETING_SITE_URL/profile (DevOps) (updated)
  • Redirect LMS_BASE_URL/dashboard/ in open edX to MARKETING_SITE_URL/dashboard (DevOps)
  • short-term, set the order history URL to the same as the dashboard URL (DevOps)
  • longer-term, upstream a PR to hide the order history item in the menu when order history URL isn't set (Open edX: hide "Order History" from the MFE user menu when it isn't configured #172)
mitxonline-theme changes
  • restore "Account" to user menu, link to MARKETING_SITE_URL/account/settings(Add account link to user menu mitxonline-theme#18)
  • Clicking on the user's name in the menu in the 'legacy' theme should take you to MARKETING_SITE/dashboard rather than LMS_BASE_URL/dashboard
frontend-component-header-edx MFE changes
@pdpinch pdpinch added the epic label Aug 20, 2021
@pdpinch pdpinch changed the title open edX: remove open edX: standardize user menus Aug 20, 2021
@pdpinch pdpinch added the DevOps label Sep 9, 2021
@asadiqbal08
Copy link
Contributor

asadiqbal08 commented Oct 1, 2021

@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
Copy link
Member Author

pdpinch commented Oct 5, 2021

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 asadiqbal08 self-assigned this Oct 6, 2021
@asadiqbal08
Copy link
Contributor

asadiqbal08 commented Oct 6, 2021

@pdpinch Here comes two questions to continue this point.

  • Are we using the fork of Learning-MFE ? , If yes then it is possible to include the fork of frontend-component-header in our forked learning MFE and import header like import Header from '@edx/frontend-component-header'; instead of import { Header } from '../course-header'; and npm aliases mechanism. For Deployment.
  • If Not using the fork of Learning-MFE, then here comes two options.
    • Create an upstream PR to Learning MFE for hiding the Order History from the menu as Tobais suggested.
    • Looked around the David's Comment for moving the course-header to frontend-component-header MFE but this change can take time to accept by OpenEdx community. My expectations are may be a week for development and testing.

@pdpinch
Copy link
Member Author

pdpinch commented Oct 6, 2021

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 edx/frontend-component-header-edx#161 and thereby hide the order history item in the menu by setting the ORDER_HISTORY_URL environment variable to null.

@briangrossman
Copy link

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

  • "Dashboard" menu item should link to: MARKETING_SITE_URL/dashboard rather than LMS_BASE_URL/dashboard/
  • "Profile" menu item should link to: MARKETING_SITE/profile rather than LMS_BASE_URL/u/USERNAME
    image
    I added these to the frontend-component-header-edx MFE changes section in the description above.

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
Copy link
Contributor

@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
Copy link
Contributor

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

@asadiqbal08
Copy link
Contributor

asadiqbal08 commented Dec 23, 2021

@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
Copy link
Contributor

@pdpinch

@asadiqbal08 asadiqbal08 removed their assignment Mar 18, 2022
@pdpinch
Copy link
Member Author

pdpinch commented Mar 25, 2022

@briangrossman is there anything left to do here?

@pdpinch
Copy link
Member Author

pdpinch commented Dec 11, 2023

@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
Copy link
Contributor

@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
Copy link
Member Author

pdpinch commented Dec 11, 2023 via email

@Ferdi
Copy link

Ferdi commented Dec 11, 2023

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

Why do they need to be consistent ?

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

No branches or pull requests

5 participants