Skip to content

Make all non-admin page-level components themeable#1042

Merged
tdonohue merged 12 commits intoDSpace:mainfrom
atmire:add-themeable-components
Mar 25, 2021
Merged

Make all non-admin page-level components themeable#1042
tdonohue merged 12 commits intoDSpace:mainfrom
atmire:add-themeable-components

Conversation

@artlowel
Copy link
Copy Markdown
Member

@artlowel artlowel commented Mar 4, 2021

References

Description

This PR makes all page-level components (i.e. components that have a URL) themeable, except the ones that are only accessible to admins.

It also adds a template themed version of each of these components to the custom theme.

Instructions for Reviewers

You can test this PR by enabling the custom them, adding your own style and/or html for the new themeable components, and verifying that your changes show up.

This PR seems bigger than it is, because it depends on #1035 You can see only the changes in this PR here

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 Difficulty: High Estimated at more than 16 hours themes labels Mar 4, 2021
@artlowel artlowel added this to the 7.0beta5 milestone Mar 4, 2021
@artlowel artlowel self-assigned this Mar 4, 2021
@lgtm-com
Copy link
Copy Markdown

lgtm-com bot commented Mar 4, 2021

This pull request introduces 4 alerts when merging ea2fae3 into e4454de - view on LGTM.com

new alerts:

  • 4 for Unused variable, import, function or class

@tdonohue tdonohue requested review from atarix83 and tdonohue March 4, 2021 16:12
@tdonohue tdonohue mentioned this pull request Mar 10, 2021
6 tasks
@tdonohue tdonohue changed the base branch from main to preview March 10, 2021 21:15
@tdonohue tdonohue changed the base branch from preview to main March 10, 2021 21:15
@DSpace DSpace deleted a comment from lgtm-com bot Mar 10, 2021
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.

@artlowel : I reviewed & tested this PR as well. It looks good & seems to work overall (no obvious issues).

However, I did want to note that even with this PR applied, I'm still seeing the same oddity on my end where the _theme_sass_variable_overrides.scss file in the custom theme doesn't appear to work. I attempted the same process that I described in your other PR & got the same results. I also tried a second browser (Firefox) just in case but saw the same results...no changes to the theme. I'm stumped as to why this doesn't work for me, but works fine on your end...but if you have any ideas on how to debug it let me know.

My environment: Windows 10, Node v14.6.0, Yarn v1.22.4

I'm not seeing any errors appear in the build process, or in Chrome DevTools when I test it. All other theme files I've tried work fine (e.g. changing CSS in _theme_css_variable_overrides.scss works perfectly), so this seems strangely specific to _theme_sass_variable_overrides.scss.

Overall, I'm +1 this PR. Just wanted to note the strange issue still exists.

@artlowel
Copy link
Copy Markdown
Member Author

@tdonohue could you try the exact commit I linked last time to see if that works for you? atmire@d212f75. Also please check the network tab in your dev tools to see that custom-theme.css is indeed being downloaded.

@LotteHofstede LotteHofstede force-pushed the add-themeable-components branch from d705572 to 0a600dd Compare March 17, 2021 10:33
@lgtm-com
Copy link
Copy Markdown

lgtm-com bot commented Mar 17, 2021

This pull request introduces 4 alerts when merging 0a600dd into 1115c58 - view on LGTM.com

new alerts:

  • 4 for Unused variable, import, function or class

@artlowel artlowel mentioned this pull request Mar 17, 2021
6 tasks
@artlowel artlowel force-pushed the add-themeable-components branch from 4a98929 to c19ee0d Compare March 18, 2021 14:47
@tdonohue tdonohue self-requested a review March 22, 2021 21:02
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.

👍 Officially approving this, as the code all looks good to me. As stated previously, the only issue I had seen is in getting the _theme_sass_variable_overrides.scss file to work as expected....but I'm still debugging what's going on there, and it can be handled in a later PR if any fix is necessary.

Thanks @artlowel !

Copy link
Copy Markdown
Contributor

@atarix83 atarix83 left a comment

Choose a reason for hiding this comment

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

thanks @artlowel for this PR
the code looks good to me (only one inline comment). I've tried to overwrite some components and it works.
I can also confirm that changing the _theme_sass_variable_overrides.scss file works for me as expected

@tdonohue
Copy link
Copy Markdown
Member

Merging, as this is at +2. Thanks @artlowel !

@tdonohue tdonohue merged commit d7daaf4 into DSpace:main Mar 25, 2021
@artlowel artlowel mentioned this pull request Jun 22, 2021
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Difficulty: High Estimated at more than 16 hours high priority themes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Support separate themes per community or collection (like XMLUI)

5 participants