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.

Sign up for GitHub

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

Jump to bottom

fix: replace the header with openedx header #327

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

fix: replace the header with openedx header #327

wants to merge 2 commits into from

Conversation

ahtesham-quraish
Copy link

@ahtesham-quraish ahtesham-quraish commented Apr 26, 2024 •
edited by syedsajjadkazmii

Description:
Replace the custom header with the Openedx header using the configurable header and plugin slots

  • Menu is prepared as props (for using configurable header)
  • edX-specific code (Dashboards user menu group and Career user menu item) is inserted via plugins
    • Plugins PR
    • edX-specific code PR

Jira Ticket
VAN-1951

How this has been tested?

  • It has been tested locally
  • New unit test is added to cover the menu feature

Screenshots

Screenshot 2024-05-29 at 12 47 32 PM Screenshot 2024-05-29 at 12 47 16 PM

@ahtesham-quraish ahtesham-quraish requested a review from a team as a code owner April 26, 2024 06:34
syedsajjadkazmii
syedsajjadkazmii reviewed Apr 26, 2024
View reviewed changes
src/containers/LearnerDashboardHeader/LearnerDashboardMenu.js Outdated Show resolved Hide resolved
src/containers/LearnerDashboardHeader/LearnerDashboardMenu.js Outdated Show resolved Hide resolved
src/containers/LearnerDashboardHeader/LearnerDashboardMenu.js Outdated Show resolved Hide resolved
src/containers/LearnerDashboardHeader/LearnerDashboardMenu.js Outdated Show resolved Hide resolved
src/containers/LearnerDashboardHeader/LearnerDashboardMenu.js Outdated Show resolved Hide resolved
src/containers/LearnerDashboardHeader/LearnerDashboardMenu.js Outdated Show resolved Hide resolved
src/containers/LearnerDashboardHeader/LearnerDashboardMenu.js Outdated Show resolved Hide resolved
src/containers/LearnerDashboardHeader/index.jsx Outdated Show resolved Hide resolved
src/containers/LearnerDashboardHeader/index.test.jsx Show resolved Hide resolved
@ahtesham-quraish ahtesham-quraish force-pushed the ahtesham/van-1914 branch 3 times, most recently from 22d3398 to 977a0a8 Compare April 29, 2024 05:22
@arbrandes arbrandes mentioned this pull request May 2, 2024
Open
@arbrandes arbrandes linked an issue May 2, 2024 that may be closed by this pull request
Change request: switch to the frontend-component-header #178
Open
@jsnwesson jsnwesson mentioned this pull request May 10, 2024
Closed
@ahtesham-quraish ahtesham-quraish force-pushed the ahtesham/van-1914 branch 2 times, most recently from e9e5ac9 to b65f44f Compare May 14, 2024 06:47
syedsajjadkazmii
syedsajjadkazmii reviewed May 14, 2024
View reviewed changes
src/containers/LearnerDashboardHeader/LearnerDashboardMenu.jsx Outdated
content: formatMessage(messages.dashboardPersonal),
isActive: true,
},
...(!_.isEmpty(dashboard) ? [{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think we should keep the condition same as it is in learner-dashboard !!dashboard.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually there was a bug on this line before, this condition "!!dashboard" was not working as per expectations we have changed it. Let us know in case any query.

image

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will let the relevant team comment on this.

@ahtesham-quraish ahtesham-quraish force-pushed the ahtesham/van-1914 branch 2 times, most recently from 5fafc13 to 7931d32 Compare May 14, 2024 09:03
@jsnwesson
Copy link
Contributor

Hey @ahtesham-quraish , I'll be looking over this PR tomorrow! A question that I do have is whether it's possible to move the edX logic of LearnerDashboardMenu.jsx to the forked Header repo (frontend-component-header-edx), which I think would help with really bringing Learner Dashboard closer to how the Header component is implemented across the MFEs.

arbrandes
arbrandes requested changes May 15, 2024
View reviewed changes
Copy link
Contributor

@arbrandes arbrandes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great in general - thanks! I'd like to see some of the business-specific code moved elsewhere, though, if we can manage it.

src/containers/LearnerDashboardHeader/LearnerDashboardMenu.jsx Outdated
},
],
userMenu: [
...(getConfig().ENABLE_EDX_PERSONAL_DASHBOARD ? [
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally, this should be moved somewhere else. We can either put the header behind a <PluginSlot /> so this 2U-specific can go in a plugin, or, as @jsnwesson comments, this could move to frontend-component-header-edx.

src/containers/LearnerDashboardHeader/LearnerDashboardMenu.jsx Outdated
{
heading: '',
items: [
...(_.isEmpty(dashboard) && getConfig().CAREER_LINK_URL ? [{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This also looks like org-specific code that we could take this opportunity to move to a plugin or a header fork.

@ahtesham-quraish
Copy link
Author

We were going with feature parity but after looking at the @arbrandes comment we are going to see the strategy to remove the 2u specific code from Dashboard MFE and move it in header-edx fork so that is why we are holding this PR back.

@syedsajjadkazmii
fix: replace the header with openedx header
857447c 
Description:
Replace the header with openedx header
van-1914
@syedsajjadkazmii syedsajjadkazmii force-pushed the ahtesham/van-1914 branch 5 times, most recently from cf00664 to c73f946 Compare May 29, 2024 08:08
@syedsajjadkazmii
Copy link
Contributor

Hey @ahtesham-quraish , I'll be looking over this PR tomorrow! A question that I do have is whether it's possible to move the edX logic of LearnerDashboardMenu.jsx to the forked Header repo (frontend-component-header-edx), which I think would help with really bringing Learner Dashboard closer to how the Header component is implemented across the MFEs.

Hi @jsnwesson, we have extracted the edX-specific code out of the learner-dashboard and now using plugin slots to insert that menu.

Could you please look at this PR and the following related PRs as well?
edx/frontend-component-header-edx#577
https://github.com/edx/edx-internal/pull/10973/files

@syedsajjadkazmii
Copy link
Contributor

syedsajjadkazmii commented May 29, 2024 •
edited

This is great in general - thanks! I'd like to see some of the business-specific code moved elsewhere, though, if we can manage it.

Hi @arbrandes, Thanks. We have removed the business-specific code from the PR. that code will be added through plugin slots in frontend-component-header-edx. Updated the PR description as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@arbrandes arbrandes arbrandes requested changes

@syedsajjadkazmii syedsajjadkazmii syedsajjadkazmii approved these changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Change request: switch to the frontend-component-header
4 participants
@ahtesham-quraish @jsnwesson @syedsajjadkazmii @arbrandes