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

Minor css variables fixes for header & navbar #2441

Conversation

alexandrevryghem
Copy link
Member

@alexandrevryghem alexandrevryghem commented Aug 16, 2023

Description

When you create a custom theme who doesn't extend the dspace theme, you can't change the background color of the header. It is also not possible to change the navbar background color using CSS variables.

Instructions for Reviewers

List of changes in this PR:

  • Made sure the default HeaderComponent in src/app now also sets the background-color: var(--ds-header-bg)
  • Made it possible to change the navbar background color with the new CSS variable --ds-navbar-bg (this is maybe not useful if you use the navbar in the same way as the dspace theme. But for custom themes who use the HeaderComponent & NavbarComponent from src/app this is useful, because the the navbar is rendered underneath the header)
  • Fixed NavbarComponent incorrectly dividing the css variable --ds-content-spacing by 3

Include guidance for how to test or review your PR.

  • Comment out the themes config in your config.yml file entirely and don't provide any themes:
    • Set the --ds-header-bg & --ds-navbar-bg in src/styles/_custom_variables.scss and check that they are correct in the UI
  • Edit your config.yml file to use the dspace theme:
    • Set the --ds-header-bg & --ds-navbar-bg in src/themes/dspace/styles/_theme_css_variable_overrides.scss and check that they are correct in the UI
    • Check that when you don't set them the UI still looks like it did before

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 Aug 16, 2023
@tdonohue tdonohue added bug port to dspace-7_x This PR needs to be ported to `dspace-7_x` branch for next bug-fix release 1 APPROVAL pull request only requires a single approval to merge labels Aug 16, 2023
@tdonohue tdonohue added this to the 7.6.1 milestone Aug 16, 2023
@tdonohue tdonohue requested a review from hardyoyo August 24, 2023 14:50
@alanorth
Copy link
Contributor

Thanks @alexandrevryghem. You are on a roll with the Angular fixes.

I use a custom theme based on the dspace theme and with this PR I will now have to theme the expandable-navbar-section in order to fix the changes to the navbar dropdowns. My header/navbar is the primary theme color, but the dropdowns are white.

Before this PR:

2023-09-29T09:39:36,203567800+03:00-fs8

After this PR:

2023-09-29T09:36:12,291482460+03:00-fs8

It's a minor issue, but I can't override the dropdown menu background without theming the expandable-navbar-section since the specificity of the component's style overrides everything else:

diff --git a/src/app/navbar/expandable-navbar-section/expandable-navbar-section.component.scss b/src/app/navbar/expandable-navbar-section/expandable-navbar-section.component.scss
index 65de77b60..1bc80d32c 100644
--- a/src/app/navbar/expandable-navbar-section/expandable-navbar-section.component.scss
+++ b/src/app/navbar/expandable-navbar-section/expandable-navbar-section.component.scss
@@ -6,6 +6,7 @@
 }
 
 .dropdown-menu {
+    background-color: var(--ds-navbar-bg);
     overflow: hidden;
     min-width: 100%;
     border-top-left-radius: 0;

So I'm not sure about this PR. I don't see the benefit in my case! 😛

@alexandrevryghem
Copy link
Member Author

alexandrevryghem commented Sep 29, 2023

@alanorth This is weird, because the expandable navbar section uses the same color as the navbar links, so they should both use --ds-navbar-link-color, so in this case they should both be white (maybe you can compare your navbar.component.scss file with the one in the dspace theme and see if there are changes regarding the color --ds-navbar-link-color). But for people who want to use different colors for the navbar vs expandable navbar, adding an extra variable is maybe the best solution.

@tdonohue tdonohue self-requested a review October 12, 2023 14:38
Copy link
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 ! Tested this and verified that header/navbar styles can now be modified via variables using either the dspace theme or custom theme. Default styling remains the same as well.

@tdonohue tdonohue merged commit be59255 into DSpace:main Oct 26, 2023
11 checks passed
@tdonohue tdonohue modified the milestones: 7.6.1, 8.0 Oct 26, 2023
@dspace-bot
Copy link
Contributor

Successfully created backport PR for dspace-7_x:

@tdonohue tdonohue removed the port to dspace-7_x This PR needs to be ported to `dspace-7_x` branch for next bug-fix release label Oct 26, 2023
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.

4 participants