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

feat: enable configurable header #560

Merged
merged 1 commit into from
Apr 23, 2024

Conversation

syedsajjadkazmii
Copy link
Contributor

@syedsajjadkazmii syedsajjadkazmii commented Mar 26, 2024

This PR makes the header configurable by accepting the custom menu in props. This replicates this feature from openedx/frontend-component-header. (Reference PR)

Details:
This PR adds the following functionality:

  • Accept the main menu in the props. if not provided, the default main menu will be used.
  • Accept the secondary menu in the props.
  • Accept the user menu in the props.
  • Restructure the user menu to add support for menu groups with heading in the user menu.
  • This will not affect the current usages of the frontend-component-header-edx.

Screenshots of using custom headers for replicating different MFE headers.
Note: Screenshots are with local setup.

Screenshot 2024-03-27 at 12 10 49 PM Screenshot 2024-03-27 at 12 11 11 PM Screenshot 2024-03-27 at 12 11 18 PM Screenshot 2024-03-27 at 12 11 34 PM Screenshot 2024-03-27 at 12 11 55 PM Screenshot 2024-03-27 at 12 12 08 PM

@syedsajjadkazmii syedsajjadkazmii marked this pull request as draft March 26, 2024 11:06
@codecov-commenter
Copy link

codecov-commenter commented Mar 26, 2024

Codecov Report

Attention: Patch coverage is 91.66667% with 4 lines in your changes are missing coverage. Please review.

Project coverage is 73.15%. Comparing base (2e76a03) to head (61a6c77).

Files Patch % Lines
src/Header.jsx 85.71% 2 Missing ⚠️
src/DesktopHeader.jsx 94.73% 1 Missing ⚠️
src/MobileHeader.jsx 93.33% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #560      +/-   ##
==========================================
+ Coverage   72.32%   73.15%   +0.82%     
==========================================
  Files          52       52              
  Lines         813      838      +25     
  Branches      158      171      +13     
==========================================
+ Hits          588      613      +25     
  Misses        215      215              
  Partials       10       10              

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

@syedsajjadkazmii syedsajjadkazmii marked this pull request as ready for review March 27, 2024 07:13
@syedsajjadkazmii syedsajjadkazmii force-pushed the sajjad/VAN-1820-configurable-header branch from 0645152 to 554e6df Compare March 27, 2024 07:14
Copy link
Contributor

@mubbsharanwar mubbsharanwar left a comment

Choose a reason for hiding this comment

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

We should add/render a configurable Header in example/index.jsx as default components are added for local testing

src/Header.jsx Show resolved Hide resolved
src/Header.jsx Show resolved Hide resolved
src/Header.jsx Outdated Show resolved Hide resolved
src/DesktopHeader.jsx Show resolved Hide resolved
src/DesktopHeader.jsx Show resolved Hide resolved
src/DesktopHeader.jsx Show resolved Hide resolved
src/MobileHeader.jsx Show resolved Hide resolved
src/Header.jsx Show resolved Hide resolved
src/MobileHeader.jsx Show resolved Hide resolved
src/DesktopHeader.jsx Show resolved Hide resolved
@syedsajjadkazmii syedsajjadkazmii force-pushed the sajjad/VAN-1820-configurable-header branch from 554e6df to 61a6c77 Compare April 2, 2024 06:39
Copy link
Contributor

@mubbsharanwar mubbsharanwar left a comment

Choose a reason for hiding this comment

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

LGTM

@syedsajjadkazmii syedsajjadkazmii force-pushed the sajjad/VAN-1820-configurable-header branch from 61a6c77 to ec6be49 Compare April 22, 2024 12:01
@syedsajjadkazmii syedsajjadkazmii merged commit 6972d0d into master Apr 23, 2024
6 checks passed
@syedsajjadkazmii syedsajjadkazmii deleted the sajjad/VAN-1820-configurable-header branch April 23, 2024 05:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants