-
Notifications
You must be signed in to change notification settings - Fork 133
Site Branding - Theme colors and logo config #4040
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
base: main
Are you sure you want to change the base?
Conversation
Some problems noted during testing (Tested on Chrome Version 132.0.6834.160 (Official Build) (arm64))
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see comments at #4040 (comment)
@milospp I'm having the same problem testing this as I am with #4039 . I use docker to test VIVO and I think the problem is that @litvinovg fix for #3034 is present in the the accompanying Vitro PR for this but not here. I tried cherry-picking the fixes into VIVO, but then ran into other problems with the Docker build. I need a clean build to test. |
@@ -35,4 +36,19 @@ | |||
an individual profile page. --> | |||
${headContent!} | |||
|
|||
<style> | |||
:root { | |||
<#if themePrimaryColorLighter?? && themePrimaryColorLighter != "null">--primary-color-lighter: ${themePrimaryColorLighter};</#if> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does the themePrimaryColorLighter != "null" check should do?
maybe use ?has_content instead of two checks ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, ?has_content is a much cleaner solution. I replaced this in the code
@@ -31,4 +31,19 @@ | |||
an individual profile page. --> | |||
${headContent!} | |||
|
|||
<style> | |||
:root { | |||
<#if themePrimaryColorLighter?? && themePrimaryColorLighter != "null">--primary-color-lighter: ${themePrimaryColorLighter};</#if> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would use ?has_content to check that variable exists, not null and not empty
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Replaced, thanks for the suggestions
I am still struggling to get this branch working for testing. Maybe rebasing from main would help. |
277e2b3
to
8208f12
Compare
@milospp I'm still getting a vitro app rather than a vivo one when I build this branch for running in Docker (using the installer/example-settings.xml) Is there a different settings file I should be using for the build? |
Ok, starting with a completely clean environment, all my docker images purged, I was finally able to get a vivo war file built out of this branch to load vivo correctly in Docker. So that's the good news. I did run into some problems with the functionality. All testing is on Apple M3 Pro Sonoma 14.7.4 (1) In Firefox (138.0.3 (aarch64)) the only way I can get the branding widget to appear is if I switch themes, click on the link to get the little popup that tells me to hit save first to appear, then click save. Any other sequence fails to do anything: https://www.awesomescreenshot.com/video/39921799?key=b598235073d45c32311257088db8c04c There are no errors in the developer console in either browser when the widget fails to display. The feature is mostly unusable in Firefox because of this. This does not happen in either Chrome or Safari. (2) Uploading a logo fails because the URL to which it’s being posted has a hardcoded path element, vivo, and when running from the root, the POST fails with a 400 error (see attached screenshot) (3) Sometimes changes made in the branding widget don’t seem to take hold. It’s hard to reproduce this reliably — It seems to happen after multiple edits to the colors. See video at https://www.awesomescreenshot.com/video/39922335?key=9b44cbc951330b7b5eb94ce311a6eb21 for an example (4) the branding widget disappears on the Capability Map tab (5) the way the primary colors selector works is still a little hard to understand from the user perspective. If I choose the base color in the middle all three colors change, whereas if I choose the lighter or darker color (top or bottom) it doesn’t affect the others. This makes sense if I know the middle color the base color but it isn’t identified as such in the display. I think it would be better to put the base color on the top and label the three boxes so that the user can understand what is happening. (6) When switching themes, the prior theme customizations are applied in the display while the theme-specific brand shows up in the branding widget. You have to save the changes in the widget for the new theme’s style to be applied. See video https://www.awesomescreenshot.com/video/39923187?key=67003a1aa1d65aad887ba89212437a4d (7) there is no way to change the the color of the text and backgrounds in the browse-by navigation menu at document.querySelector("#browse-by > nav") (8) the branding widget also disappears on the editing displays. See video |
![]()
|
Does anyone know of a better way to handle updates on theme change? /api/src/main/java/edu/cornell/mannlib/vedit/controller/OperationController.java
This resolves Bridget's bullet 6. |
Regarding (1) the firefox issue - I have tried removing my cached data, running in incognito mode, and cannot get this to work no matter what I do. Here's the error I see in the console - it seems to have trouble loading the theme-config.json, I have verified that I can load it manually (e.g. at http://localhost:8080/themes/wilma/theme-config.json) and it clearly is there. ![]() If others are able to get this to work in Firefox, I guess we could assume it is an odd problem with my environment and move on, but it would be good to have at least a couple of people check that. All of the other problems appear fixed Regarding the solution for (6), switching themes, I agree that the solution you implemented isn't ideal. I don't have any other good ideas unfortunately. But I also don't think this is a critical issue, since it doesn't really seem to be a very likely use case anyway. It might be better to just document the behavior than to insert that cache clearing code there. I would like at least one other person to verify that the functionality works as it should in Firefox before I approve the PR. |
I was able to reproduce this bug in Firefox and believe I’ve resolved it. Could you please validate it again? Apologies for the inconvenience, and thank you! |
Ok, getting closer :-). the fix does address the problem with the branding widget not displaying in Firefox. I have found 2 additional issues, one specific to Firefox, and one which was probably introduced by a recent change. Continuing with previous numbering to avoid confusion.
|
Fixed 9.1 Fixed alert message 9.2: Added a fallback save mechanism:
9.3 Addressed Firefox color picker limitations: On Windows: Platform behavior summary: These behaviors cannot be handled with JavaScript, as the native picker opens in a separate OS-level window.
Explanation:
https://www.awesomescreenshot.com/video/40160534?key=3f83734eb8794f9479f7f70b5638be99 If you have any suggestions or concerns about this behavior, feel free to share them! |
Re (10) since we can't distinguish easily between a user actively setting the lighter/darker color and it being automatically set for them by choosing a base color, there isn't a great solution here in IMHO. Most people will be able to figure out if they click Reset they can start over with the colors so I guess I'm fine with what you have coded. It could be better but it could also be worse. Re (9) I think your fix to ensure the changes are saved or canceled as appropriate is also good enough for now. It's definitely not worth writing our own color picker at this point. I will add my approval for this PR now (and the accompanying Vitro PR which is really where I should probably have been adding all of these comments..) |
@milospp yes that’s a good idea. I like the 3rd option you have there the best. With the link displayed vertically between the base and darker color. |
066a03c
to
5dfa079
Compare
VIVO GitHub issue
Linked Vitro PR
What does this pull request do?
This pull request allows administrators to customize institutional branding by defining institutional colors and updating the key color scheme of the current theme.
What's new?
Administrators can now change the following theme colors:
Additional options for detailed customization:
Screenshoots
Regular Color Settings
Advanced Color Settings
Reset Color Option
How should this be tested?
General Testing
Ensure testing is conducted for every available theme.
Test 1: Theme Colors
Test 2: Logo Upload
Test 3: Logo Small Upload
Test 4: Only one logo uploaded
Test 5: Logo Removal