Skip to content

Fixed mobile navbar not expanding & use same css variable for all header icons#2220

Merged
tdonohue merged 2 commits intoDSpace:mainfrom
alexandrevryghem:fix-mobile-navbar-and-header-icon-css-variables
May 9, 2023
Merged

Fixed mobile navbar not expanding & use same css variable for all header icons#2220
tdonohue merged 2 commits intoDSpace:mainfrom
alexandrevryghem:fix-mobile-navbar-and-header-icon-css-variables

Conversation

@alexandrevryghem
Copy link
Copy Markdown
Member

Description

Currently the mobile navbar does not open on the home page and on other pages like the item page the navbar is not being shown correctly for both the dspace and the custom theme.
This PR also makes it easier to customise the color and the hover color from the header icons by using the same variable for all of them (using the css variables --ds-header-icon-color & --ds-header-icon-color-hover).

Instructions for Reviewers

List of changes in this PR:

  • HeaderNavbarWrapperComponent: applied the --ds-nav-z-index on the wrapper
  • Applied the css variables --ds-header-icon-color & --ds-header-icon-color-hover on every header icon

Guidance for how to test and review this PR:

  • Uncomment the custom theme in src/themes/eager-themes.module.ts
  • For both the dspace and the custom theme check the following:
    • Add these lines to the theme's _theme_css_variable_overrides.scss:
      --ds-header-icon-color: red;
      --ds-header-icon-color-hover: orange;
      
    • Check that all the header icons are now red by default and orange when you hover/focus on them. Check this in both desktop mode and mobile mode and check this when you are logged in and also check it when you are logged out.
    • Check that the mobile navbar can be expanded

Checklist

  • My PR is small in size (e.g. less than 1,000 lines of code, not including comments & specs/tests), or I have provided reasons as to why that's not possible.
  • My PR passes ESLint validation using yarn lint
  • My PR doesn't introduce circular dependencies (verified via yarn check-circ-deps)
  • My PR includes TypeDoc comments for all new (or modified) public methods and classes. It also includes TypeDoc for large or complex private methods.
  • My PR passes all specs/tests and includes new/updated specs or tests based on the Code Testing Guide.
  • If my PR includes new libraries/dependencies (in package.json), I've made sure their licenses align with the DSpace BSD License based on the Licensing of Contributions documentation.
  • If my PR includes new features or configurations, I've provided basic technical documentation in the PR itself.
  • If my PR fixes an issue ticket, I've linked them together.

@alexandrevryghem alexandrevryghem self-assigned this May 1, 2023
@alexandrevryghem alexandrevryghem force-pushed the fix-mobile-navbar-and-header-icon-css-variables branch from c420cf2 to c042a5c Compare May 1, 2023 12:43
@tdonohue tdonohue added bug themes 1 APPROVAL pull request only requires a single approval to merge labels May 1, 2023
@tdonohue tdonohue added this to the 7.6 milestone May 1, 2023
@tdonohue tdonohue self-requested a review May 4, 2023 14:33
Copy link
Copy Markdown
Member

@tdonohue tdonohue left a comment

Choose a reason for hiding this comment

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

👍 Thanks @alexandrevryghem ! I was able to reproduce the mobile navbar bug on main. I've verified this fixes the bug (for both themes) and verified the CSS variables for header icons work (again for both themes).

@tdonohue tdonohue merged commit 7e3613f into DSpace:main May 9, 2023
4science-it pushed a commit to 4Science/dspace-angular that referenced this pull request Sep 9, 2024
[DSC-1902] enable edit and new menu sections for community and collections admin

Approved-by: Vincenzo Mecca
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

1 APPROVAL pull request only requires a single approval to merge bug themes

Projects

No open projects
Status: ✅ Done

Development

Successfully merging this pull request may close these issues.

2 participants