Site Branding - Theme colors and logo config#4040
Site Branding - Theme colors and logo config#4040milospp wants to merge 18 commits intovivo-project:mainfrom
Conversation
|
Some problems noted during testing (Tested on Chrome Version 132.0.6834.160 (Official Build) (arm64))
|
|
@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. |
|
|
||
| <style> | ||
| :root { | ||
| <#if themePrimaryColorLighter?? && themePrimaryColorLighter != "null">--primary-color-lighter: ${themePrimaryColorLighter};</#if> |
There was a problem hiding this comment.
What does the themePrimaryColorLighter != "null" check should do?
maybe use ?has_content instead of two checks ?
There was a problem hiding this comment.
Yes, ?has_content is a much cleaner solution. I replaced this in the code
|
|
||
| <style> | ||
| :root { | ||
| <#if themePrimaryColorLighter?? && themePrimaryColorLighter != "null">--primary-color-lighter: ${themePrimaryColorLighter};</#if> |
There was a problem hiding this comment.
I would use ?has_content to check that variable exists, not null and not empty
There was a problem hiding this comment.
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 |
Also this functionality will work only with default colors, if user manually tweak lighter color it will not overide his choice (unless he reset color)
|
|
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
|
I think this is working pretty well right now. There are still a few elements that don't get changed with the branding (e.g. the tab background colors and the reset button on the Capability Map page), but frankly it's probably good enough for now. I also noticed that in Chrome at least when you upload new logos you have to clear your browser cache for them to display. Tested in Chrome, Firefox and Safari. |
The Reset button on the Capability Map page is styled in red to represent a "danger" action. This color should always remain red and must not be overridden by primary, secondary, or accent colors. If someone wants to change it, a CSS update will be required. |
|
I think we should just leave this as-is. Thank you for the explanations @milospp . No further requests for changes from me. |
a8138c9 to
032cf8f
Compare
|
Doesn't work for me for some reason. |
|
I see 404 errors while requesting theme-config.json and cssUploadUtils.js |
|
Can you explain in detail when exactly this happened? Have you tried with another browser? I recently changed the configuration structure stored in localStorage. If there are old values, they may be causing the issue. To handle this, I added a try–catch block that invalidates the config if the script fails to load. |
|
When I click on "Change theme branding colors" nothing is happening and I see errors in browser console. |
|
Can you check if you have that file in the build target location: During build, it should be copied from: |
You are right, VIVO part wasn't deployed correctly. Thanks! |
|
@milospp Could you take a look why logo's aren't displayed on any theme but Nemo? |
|
Logo appears on tenderfoot and wilma. |














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:
The theme editor is fully customizable per theme.
Each theme’s colors are defined in:
webapp/themes/{theme}/theme-config.jsonScreenshoots
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