-
Notifications
You must be signed in to change notification settings - Fork 85
Created custom css style upload #485
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
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.
On the whole, this works as documented.
The CSS links on the Style upload page are broken when VIVO is deployed at the root and not /vivo.
Also there is some security risk with this feature as a whole, since it's possible that CSS injection could be used in the uploaded css file (or potentially other malware). I suppose this is mitigated by the fact that only an admin has access to the page, but it still feels worth pointing out.
I wonder if it would be better to allow the user to specify a remote url for css rather than uploading the file locally.
@@ -0,0 +1,130 @@ | |||
<div> | |||
<h2>Site Styles</h2> | |||
<div class="button-group"> |
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.
These links don't work if VIVO is deployed at the root instead of at /vivo
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.
Great catch, I'll fix this right away
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.
With the fix to the css links in place, I'm good with this PR now.
@@ -469,6 +476,9 @@ protected Map<String, Object> getPageTemplateValues(VitroRequest vreq) { | |||
|
|||
// Add these accumulator objects. They will collect tags so the template can write them | |||
// at the appropriate location. | |||
|
|||
map.putAll(getCustomCss()); |
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 think instead you could create a static String field initialized with the same code and updated by SiteStyleController.
<a class="button blue" download href="${urls.base}/css/home-page-maps.css">page-createAndLink.css</a> | ||
|
||
|
||
<#if themeDir == "themes/wilma"> |
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.
Hardcoding theme paths is not an option. It will be broken once theme is renamed for customization.
webapp/src/main/webapp/templates/edit/specific/applicationBean_retry.jsp
Outdated
Show resolved
Hide resolved
webapp/src/main/webapp/templates/freemarker/body/siteAdmin/siteAdmin-siteStyle.ftl
Outdated
Show resolved
Hide resolved
webapp/src/main/webapp/templates/freemarker/body/siteAdmin/siteAdmin-siteStyle.ftl
Outdated
Show resolved
Hide resolved
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.
This seems to work well enough. My review is mostly of the functionality and not of the code itself. It would be good to have someone with more familiarity with the VIVO code to confirm the code. I did have a couple of minor suggestions for wording changes for clarity but they aren't critical.
This reverts commit 7a11a44.
38f23c5
to
3e3c797
Compare
@@ -20,6 +20,9 @@ | |||
import org.apache.commons.lang3.StringUtils; | |||
import org.apache.commons.logging.Log; | |||
import org.apache.commons.logging.LogFactory; | |||
import org.apache.jena.ontology.OntModel; | |||
import org.apache.jena.rdf.model.*; |
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.
Could you fix wildcard import here?
@@ -6931,3 +6931,34 @@ uil-data:there_are_no_results_to_display.Vitro | |||
uil:hasKey "there_are_no_results_to_display" ; | |||
uil:hasPackage "Vitro-languages" . | |||
|
|||
uil-data:back.Vitro |
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 think it is not used.
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.
Missed that one to remove. Fixed
|
||
.button.red { | ||
background-color: #b40000 | ||
} |
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.
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.
|
||
displayModel.removeAll(portalResource, property, null); | ||
|
||
if (!cssFilePath.isEmpty() && !cssFilePath.equals("null")) { |
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.
Is there a way to avoid "null" special value?
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.
It looks like this isn't used, I initially added it to improve the robustness of the function, but on closer inspection, it seems that workflow is never going to happen.
Removed that check
private void showMainStyleEditPage(HttpServletRequest request, HttpServletResponse response, String info) { | ||
try { | ||
response.setContentType("text/html"); | ||
response.getWriter().println("<html><body><h2>Site Style</h2><pre>customCssUrl: " |
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.
Is this used somewhere?
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.
This is intended for direct API calls to reset CSS. It should not be used in regular workflows—only as a fallback solution.
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.
How this html is used in direct API calls? I couldn't find the code.
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.
To check what file is uploaded
http://localhost:8080/vivo/siteStyle
To remove file (works only if admin is logged in)
http://localhost:8080/vivo/siteStyle?action=remove
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.
This is the same request that is sent when the user clicks on the remove button, but in case somehow this is not available, this is a workaround
} | ||
|
||
|
||
private void removeCssFileDisplayModel() { |
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.
Uploaded css style file is not deleted on removal.
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.
File related triples should be removed too.
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.
Added deleting file on replace or remove CSS
} | ||
|
||
function resetStyles() { | ||
if (confirm('Are you sure you want to remove the custom CSS?')) { |
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.
Text should be in translation files I think
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.
Resolved, added to i18n
…ransaltions and styles
is = file.getInputStream(); | ||
mediaType = tika.detect(is); | ||
} catch (IOException e) { | ||
log.error(e.getLocalizedMessage()); |
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 suggest using log.error(e, e)
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.
Repalced
try { | ||
fileInfo = fileHelper.createFile(storedFileName, getMediaType(file), file.getInputStream()); | ||
} catch (Exception e) { | ||
log.error(e.getLocalizedMessage()); |
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 suggest using log.error(e, e)
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.
Repalced
return confirm(msg); | ||
} | ||
|
||
async function onSave(e) { |
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.
As this new js code is page specific it would be better to move js code in a separate file to be only applicable to one page.
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.
Since formBasic (and scripts are generally included via controller)
Line 88 in aeee7e7
request.setAttribute("scripts","/templates/edit/formBasic.js"); |
Should I replace this formBasic.js
with my custom JS or include it inside the .ftl document?
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.
Could new js be added in applicationBean_retry.jsp ?
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 can add a <script> tag inside applicationBean_retry.jsp
, which would inject the script into the body. This approach works technically, and it aligns with how scripts are already being included in some other parts of VIVO. However, my only concern is whether it's clean practice to place script tags in the body instead of the head, as is typically recommended.
Currently in formBasic.js
, there is only this code, which I think is not used in this jsp page, therefore I can also replace the script inside Controller without problems.
<!-- $This file is distributed under the terms of the license in LICENSE$ -->
<script language="JavaScript" type="text/javascript">
function confirmDelete() {
var msg="Are you SURE you want to delete this record? If in doubt, CANCEL."
return confirm(msg);
}
</script>
What do you think?
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.
if it is not used on the page, then sure it should be easier to just replace formBasic.js with new js filename.
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.
Split into separate parts. Please check if I didn’t accidentally introduce any issues.
VIVO GitHub issue
Linked Vivo PR
What does this pull request do?
This pull request enables administrators to manage custom CSS styles directly from the Site Admin page. Admins can upload, download, and remove custom CSS styles, as well as reset styles to ensure accessibility and functionality remain intact.
What's new?
Additional Notes
This PR introduces ReferrerHelper.java
This util class easily retrieves the previous page from the request and uses it for the back button.
The traditional approach for handling the back button was to always navigate to the document.referrer or previously visited page. However, this strategy breaks when dealing with nested navigation flows.
Home -> Page 2 -> Page 3
when the user is on Page 3 and clicks the back button, they are correctly taken back to Page 2. But at that point, the referrer becomes Page 3 again. So if the user clicks back on Page 2, they are taken to Page 3—creating an infinite loop between Page 2 and Page 3.
This ReferrerHelper class mitigates the problem by preventing the referrer from being overwritten in such nested navigation flows.
Screenshoots
Style edit page
Example css
Broken contrast css (RESET button is still visible)
How should this be tested?
Test for every theme
Uploading regular CSS
Expected Result: The site background should change to red.
Download Custom CSS
Expected Result: The downloaded file should match the uploaded custom CSS.
Removing Custom CSS
After uploading custom CSS, navigate to the Change CSS Styles page.
Click Remove Custom CSS.
Expected Result: The site should revert to the default styles.
Uploading broken CSS
Example:
or
Reset button should still be visible and working
Additional Notes:
The custom CSS file is saved using the Vitro FileStorage class and file is stored into vivo-home/uploads.
The file's path is stored in the ApplicationBean under the key customCssPath (vitro-kb-applicationMetadata).