fix: replace the header with openedx header #327

ahtesham-quraish · 2024-04-26T06:34:35Z

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

edX-specific code (Dashboards user menu group and Career user menu item) is inserted via plugins
- edX Header PR
- edX-internal PR
- Dashboard plugin 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

Menu	Screenshot
Career
Dasbboard
Enterprose Dasbboard

src/containers/LearnerDashboardHeader/LearnerDashboardMenu.js

src/containers/LearnerDashboardHeader/index.jsx

src/containers/LearnerDashboardHeader/index.test.jsx

syedsajjadkazmii · 2024-05-14T07:16:05Z

src/containers/LearnerDashboardHeader/LearnerDashboardMenu.jsx

+            content: formatMessage(messages.dashboardPersonal),
+            isActive: true,
+          },
+          ...(!_.isEmpty(dashboard) ? [{


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

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.

I will let the relevant team comment on this.

jsnwesson · 2024-05-14T21:11:42Z

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

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.

arbrandes · 2024-05-15T12:43:27Z

src/containers/LearnerDashboardHeader/LearnerDashboardMenu.jsx

+    },
+  ],
+  userMenu: [
+    ...(getConfig().ENABLE_EDX_PERSONAL_DASHBOARD ? [


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.

arbrandes · 2024-05-15T12:45:20Z

src/containers/LearnerDashboardHeader/LearnerDashboardMenu.jsx

+    {
+      heading: '',
+      items: [
+        ...(_.isEmpty(dashboard) && getConfig().CAREER_LINK_URL ? [{


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

ahtesham-quraish · 2024-05-16T06:22:18Z

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 · 2024-05-29T09:00:03Z

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 · 2024-05-29T09:01:04Z

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.

jsnwesson · 2024-07-03T16:43:02Z

src/containers/LearnerDashboardHeader/hooks.js

+  courseSearchUrl, authenticatedUser, dashboard, exploreCoursesClick,
+}) => {
+  const { formatMessage } = useIntl();
+  return getLearnerHeaderMenu(formatMessage, courseSearchUrl, authenticatedUser, dashboard, exploreCoursesClick);


I'm noticing that dashboard never gets used by getLearnerHeaderMenu, and it's supposed to be data from reduxHooks.useEnterpriseDashboardData() in LearnerDashboardHeader/index.jsx. Should some of this logic live in the 2U-forked header-component?

yes, you are right. The dashboard is removed from here and edx specific login has been moved out.

codecov · 2024-07-25T11:00:22Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.72%. Comparing base (68a46ac) to head (dc2ae40).

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #327      +/-   ##
==========================================
+ Coverage   95.70%   95.72%   +0.01%     
==========================================
  Files         166      163       -3     
  Lines        1491     1451      -40     
  Branches      268      244      -24     
==========================================
- Hits         1427     1389      -38     
+ Misses         60       58       -2     
  Partials        4        4

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

syedsajjadkazmii · 2024-08-05T09:17:10Z

Hi @jsnwesson @arbrandes,

this PR is ready for re-review. Please have a look.

jsnwesson · 2024-08-06T21:50:54Z

Thanks @syedsajjadkazmii ! I tested out the Learner Dashboard locally with the other PRs you linked above, and this checks out for me.

arbrandes

I've (finally) tested this, and it works fine! Code looks good, too. Thanks a lot for doing this!

arbrandes · 2024-08-09T20:23:38Z

@brian-smith-tcril, FYI: learner dashboard will now have a standard header, which should make the slot work a little easier.

Description: Replace the header with openedx header van-1914

ahtesham-quraish requested a review from a team as a code owner April 26, 2024 06:34

ahtesham-quraish force-pushed the ahtesham/van-1914 branch from ec9abb1 to 418657e Compare April 26, 2024 06:34

syedsajjadkazmii reviewed Apr 26, 2024

View reviewed changes

ahtesham-quraish force-pushed the ahtesham/van-1914 branch 3 times, most recently from 22d3398 to 977a0a8 Compare April 29, 2024 05:22

arbrandes mentioned this pull request May 2, 2024

Change request: switch to the frontend-component-header #178

Closed

arbrandes linked an issue May 2, 2024 that may be closed by this pull request

Change request: switch to the frontend-component-header #178

Closed

ahtesham-quraish force-pushed the ahtesham/van-1914 branch from 977a0a8 to a8274a8 Compare May 3, 2024 05:41

jsnwesson mentioned this pull request May 10, 2024

feat: display profile name in navbar if available #313

Closed

ahtesham-quraish force-pushed the ahtesham/van-1914 branch 2 times, most recently from e9e5ac9 to b65f44f Compare May 14, 2024 06:47

syedsajjadkazmii reviewed May 14, 2024

View reviewed changes

syedsajjadkazmii approved these changes May 14, 2024

View reviewed changes

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

arbrandes requested changes May 15, 2024

View reviewed changes

syedsajjadkazmii force-pushed the ahtesham/van-1914 branch 6 times, most recently from c73f946 to fb35614 Compare May 29, 2024 08:57

jsnwesson reviewed Jul 3, 2024

View reviewed changes

deborahgu added the waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. label Jul 12, 2024

mubbsharanwar force-pushed the ahtesham/van-1914 branch from fb35614 to eddb511 Compare July 25, 2024 10:30

mubbsharanwar force-pushed the ahtesham/van-1914 branch from b31c7d0 to 69f813c Compare July 25, 2024 10:55

mubbsharanwar mentioned this pull request Jul 29, 2024

feat: add user menu slots in header edx/frontend-component-header-edx#577

Merged

mubbsharanwar force-pushed the ahtesham/van-1914 branch 4 times, most recently from 7a295a2 to 2d4db56 Compare August 5, 2024 07:20

abdullahwaheed requested a review from arbrandes August 8, 2024 06:40

mubbsharanwar force-pushed the ahtesham/van-1914 branch from 2d4db56 to d7bec07 Compare August 9, 2024 09:49

arbrandes approved these changes Aug 9, 2024

View reviewed changes

brian-smith-tcril mentioned this pull request Aug 9, 2024

Allow Header Customization via Frontend Plugin Framework openedx/frontend-component-header#521

Open

mubbsharanwar force-pushed the ahtesham/van-1914 branch from d7bec07 to 4914061 Compare August 12, 2024 06:36

fix: replace the header with openedx header

dc2ae40

Description: Replace the header with openedx header van-1914

mubbsharanwar force-pushed the ahtesham/van-1914 branch from 4914061 to dc2ae40 Compare August 12, 2024 06:44

mubbsharanwar merged commit 7ef5d5b into master Aug 12, 2024
7 checks passed

mubbsharanwar deleted the ahtesham/van-1914 branch August 12, 2024 06:55

jsnwesson removed the waiting for eng review PR is ready for review. Review and merge it, or suggest changes. label Aug 14, 2024

MaxFrank13 mentioned this pull request Sep 16, 2024

fix: if support page is not set, help button does not appears in header #445

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: replace the header with openedx header #327

fix: replace the header with openedx header #327

ahtesham-quraish commented Apr 26, 2024 •

edited by mubbsharanwar

Loading

syedsajjadkazmii May 14, 2024

ahtesham-quraish May 14, 2024 •

edited

Loading

syedsajjadkazmii May 14, 2024

jsnwesson commented May 14, 2024

arbrandes left a comment

arbrandes May 15, 2024

arbrandes May 15, 2024

ahtesham-quraish commented May 16, 2024

syedsajjadkazmii commented May 29, 2024

syedsajjadkazmii commented May 29, 2024 •

edited

Loading

jsnwesson Jul 3, 2024

syedsajjadkazmii Jul 31, 2024

codecov bot commented Jul 25, 2024 •

edited

Loading

syedsajjadkazmii commented Aug 5, 2024

jsnwesson commented Aug 6, 2024

arbrandes left a comment

arbrandes commented Aug 9, 2024

fix: replace the header with openedx header #327

fix: replace the header with openedx header #327

Conversation

ahtesham-quraish commented Apr 26, 2024 • edited by mubbsharanwar Loading

syedsajjadkazmii May 14, 2024

Choose a reason for hiding this comment

ahtesham-quraish May 14, 2024 • edited Loading

Choose a reason for hiding this comment

syedsajjadkazmii May 14, 2024

Choose a reason for hiding this comment

jsnwesson commented May 14, 2024

arbrandes left a comment

Choose a reason for hiding this comment

arbrandes May 15, 2024

Choose a reason for hiding this comment

arbrandes May 15, 2024

Choose a reason for hiding this comment

ahtesham-quraish commented May 16, 2024

syedsajjadkazmii commented May 29, 2024

syedsajjadkazmii commented May 29, 2024 • edited Loading

jsnwesson Jul 3, 2024

Choose a reason for hiding this comment

syedsajjadkazmii Jul 31, 2024

Choose a reason for hiding this comment

codecov bot commented Jul 25, 2024 • edited Loading

Codecov Report

syedsajjadkazmii commented Aug 5, 2024

jsnwesson commented Aug 6, 2024

arbrandes left a comment

Choose a reason for hiding this comment

arbrandes commented Aug 9, 2024

ahtesham-quraish commented Apr 26, 2024 •

edited by mubbsharanwar

Loading

ahtesham-quraish May 14, 2024 •

edited

Loading

syedsajjadkazmii commented May 29, 2024 •

edited

Loading

codecov bot commented Jul 25, 2024 •

edited

Loading