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

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

Open
dyudyunov opened this issue Jul 14, 2023 · 13 comments · May be fixed by #327
Open

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

dyudyunov opened this issue Jul 14, 2023 · 13 comments · May be fixed by #327
Labels
enhancement Relates to new features or improvements to existing features
Milestone

Comments

@dyudyunov
Copy link

The problem

Instead of using the frontend-component-header the MFE uses its own LearnerDashboardHeader component. Main pain points here:

  • it’s a third header on the platform (including the legacy header and the header component used by other MFEs) which is confusing for users
  • it has hardcoded edx’s values, like the edx logo, which I believe will be a violation of the trademark policies if used elsewhere
    image

Proposal

It would be great to replace it with the easily-brandable frontend-component-header.

Additional info

@adamstankiewicz
Copy link
Member

[inform] Just dropping some helpful links for whenever this issue gets picked up:

@mariajgrimaldi
Copy link
Member

mariajgrimaldi commented Nov 3, 2023

This was partially solved by: #205, which makes it customizable by adding an environment variable. Which value is read during build time. So we would need to add support for the MFE Config for a more flexible solution as Adam mentioned. Although I'm wondering why we are using a custom header instead of the LearningHeader -- or a similar version as suggested in this issue? Which sounds like the best long-term solution but with lots of work.

@mariajgrimaldi
Copy link
Member

mariajgrimaldi commented Nov 7, 2023

Here's a PR with the suggested change (getting the logo from the MFE API): #238

@jsnwesson
Copy link
Contributor

Hey all, just wanted to jump in to say that my team (@openedx/2u-aperture, soon-to-be-maintainers) and I fully support replacing the header with the default MFE Header. We'll definitely want to know when that change is ready to happen, but as a heads up we'd want to ensure two things in order to accept the PR:

  1. The "Careers" option still appears for edX.org users (original PR with screenshots, fair warning that there is a bug somewhere that is hiding this)
  2. The learner will still be able to toggle between their dashboard or one of the available Enterprise dashboards via the modal or dropdown.
Screenshot 2023-11-08 at 1 42 42 PM Screenshot 2023-11-08 at 1 42 48 PM

@mariajgrimaldi
Copy link
Member

@jsnwesson: thanks for the details! I don't know how much work it would be to change the header, but when we have it done, is it enough to have flags to turn on/off both dashboards? Unless we have a replacement for the community, I'll tag @arbrandes here. Should we talk with the product wg for this one?

@jsnwesson
Copy link
Contributor

@mariajgrimaldi I would say it's enough to have flags for the dual dashboard feature!
I'm not involved as much with the argument for certain Header implementations over others, but I would imagine that if we ever want to make it so the Header and Footer can be wrapped around all MFEs (Piral being one method that comes to mind), then we'd want to stick with the same Header/Footer dependencies across the board.

@arbrandes arbrandes added this to the Redwood.1 milestone Dec 6, 2023
@arbrandes arbrandes added the enhancement Relates to new features or improvements to existing features label Feb 9, 2024
@hinakhadim
Copy link
Contributor

Hi everyone, Just want to know if anyone is actively working on this at the moment? or Are we paused because of "Careers" option and dual dashboard thing.
What if we'll include both things in @edx/frontend-component-header and use that header package in all MFEs for UI consistency or is there any problem in that? Looking forward to your thoughts on this so that I can create PR.

@jsnwesson
Copy link
Contributor

Hi @hinakhadim ! As it turns out, I believe the @openedx/2u-vanguards team is planning on bringing in the customized header into @edx/frontend-component-header. @zainab-amir is that still accurate?

@0x29a
Copy link

0x29a commented Apr 23, 2024

Hi @zainab-amir and @openedx/2u-vanguards, just wanted to know whether what @jsnwesson wrote above is correct and if anyone is working on this at the moment? One of our clients is interested in making the header look consistent across all MFEs, so depending on what needs to be done, we could probably create a PR as well or help with an existing one if needed. Looking forward to your thoughts.

@syedsajjadkazmii
Copy link
Contributor

syedsajjadkazmii commented Apr 26, 2024

Hi @0x29a, yes its correct. We have launched configurable header feature in v5.2.0 and we are currently working on replacing custom header with frontned-component-header in learner-dashboard.

cc: @ahtesham-quraish

@0x29a
Copy link

0x29a commented Apr 26, 2024

That's awesome, @syedsajjadkazmii! Thank you and your team for working on this. Please let us know if we can help anyhow.

@arbrandes
Copy link
Contributor

This is the relevant PR, if I'm not mistaken:

#327

@arbrandes arbrandes linked a pull request May 2, 2024 that will close this issue
@zainab-amir
Copy link
Contributor

@arbrandes yes, that is the PR to replace the learner dashboard custom header with frontend-component-header. We have a small followup PR that needs your review: openedx/frontend-component-header#494

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Relates to new features or improvements to existing features
Projects
Status: In progress
Development

Successfully merging a pull request may close this issue.

9 participants