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

Use CSS variables in TypeScript #1853

Merged

Conversation

LotteHofstede
Copy link
Contributor

@LotteHofstede LotteHofstede commented Sep 22, 2022

Description

This PR restores the feature we used to have to read out CSS variables from TypeScript. With the upgrade to Angular 8 we had to hardcode those values (so they would no longer update automatically if they changed in CSS). This PR bases them on CSS again. Because by now all our variables are CSS variables, it also gives us a lot more info than we used to have.

Its implementation is based on this article.
The variables are saved in the store when the page is loaded.

Any references to existing variables used were updated to use the correct CSS variables.

Instructions for Reviewers

After the application has started, all our custom and bootstrap CSS variables should be visible in the store under "cssVariables". (Using Redux DevTools).

The easiest way to test this PR would be to:

$grid-breakpoints: (
  xs: 0,
  sm: 990px,
  md: 991px,
  lg: 992px,
  xl: 1200px
) !default;

When you run the app now, you should see 990px in app component; the version from the theme.

You can do the same thing on the current main branch, only there that same variable is named slighty different, so add <h1>{{ cssService.getVariable('smMin') | async }}</h1> to the app component html file instead. On main you'll see that the value doesn't change when you change the css file. Instead you'll see the hardcoded version

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 TSLint validation using yarn run lint
  • My PR doesn't introduce circular dependencies
  • 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, third-party dependencies (in package.json), I've made sure their licenses align with the DSpace BSD License based on the Licensing of Contributions documentation.

@artlowel artlowel added this to Needs Reviewers Assigned in DSpace 7.4 release via automation Sep 22, 2022
@artlowel artlowel added this to the 7.4 milestone Sep 22, 2022
@tdonohue tdonohue self-requested a review September 22, 2022 15:03
@tdonohue tdonohue added the 1 APPROVAL pull request only requires a single approval to merge label Sep 22, 2022
@tdonohue tdonohue moved this from Needs Reviewers Assigned to Under Review in DSpace 7.4 release Sep 22, 2022
@tdonohue tdonohue modified the milestones: 7.4, 7.5 Oct 4, 2022
@tdonohue tdonohue removed this from Under Review in DSpace 7.4 release Oct 4, 2022
@tdonohue tdonohue added this to Needs Reviewers Assigned in DSpace 7.5 release (OLD - Migrated, see note above) via automation Oct 4, 2022
@tdonohue tdonohue removed this from Needs Reviewers Assigned in DSpace 7.5 release (OLD - Migrated, see note above) Oct 11, 2022
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 @LotteHofstede . This looks good to me. I gave it some light testing by changing the values of the CSS variables used in typescript. I also agree this is much better than the current hardcoded approach in app.component.ts

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.

None yet

3 participants