-
Notifications
You must be signed in to change notification settings - Fork 131
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
Replace Qtip Library with Floating ui #3937
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.
Works well with Wilma, excellent! I'm seeing an error with tenderfoot, however.
org.apache.tomcat.util.threads.TaskThread$WrappingRunnable.run(TaskThread.java:61) at java.base/java.lang.Thread.run(Thread.java:840) Caused by: freemarker.core.ParseException: Syntax error in template "individual--foaf-person.ftl" in line 184, column 1: Encountered ")", but was expecting one of: <STRING_LITERAL> <RAW_STRING> "false" "true" <INTEGER> <DECIMAL> "." "+" "-" "!" "[" "(" "{" <ID> at
Seems there's an extra comma in that file. If I remove the comma the page loads, but it then complains about missing methods in the console, which I can resolve by adding tooltip-utils.js and popper.min.js to headscripts.ftl... but then it complains about bootstrap missing, which is as deep as I got :)
Seems like we should not break tenderfoot if we intend to continue distributing it with VIVO (are we...?)
I'm also not seeing any tooltips with nemo, but they seem broken without the changes, as well, so not worried about fixing that theme with this PR.
webapp/src/main/webapp/themes/tenderfoot/templates/body/individual/individual--foaf-person.ftl
Show resolved
Hide resolved
Completely overlooked testing on other themes since the decision to prioritize Wilma and phase out the others in the future. |
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.
Some suggestions after looking at the code. Didn't test the PR yet.
Did I get it right that this pull request break popups for other themes (in case user installed VIVO 1.13 and copied wilma/tenerfoot with new name to create custom theme for his instance)?
webapp/src/main/webapp/js/visualization/mapofscience/VisCommonControl.js
Outdated
Show resolved
Hide resolved
The popup library is changed and applied to every theme. |
This PR was opened after the Wilma theme update which included migration to Bootstrap 5, therefore this PR requires Bootstrap 5 for Popper to work. Meanwhile, The Wilma update was reverted from the main branch, so I had to include the Bootstrap 5 files in this PR. |
…roperty control panel buttons
webapp/src/main/webapp/themes/tenderfoot/templates/page/partials/headScripts.ftl
Outdated
Show resolved
Hide resolved
@@ -273,6 +273,11 @@ a.map-of-science-links { | |||
color: #2485AE; | |||
} | |||
|
|||
.visCommonControllTooltip { |
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 it needed? Seems like the same css as in webapp/src/main/webapp/css/visualization/mapofscience/layout.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.
webapp/src/main/webapp/css/visualization/mapofscience/layout.css
This CSS is loaded only in Vitro theme and
webapp/src/main/webapp/themes/nemo/css/layout.css
this CSS is loaded only in the Nemo theme
so this should be in both file
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 tried to take a look how map of science looks on Nemo theme, but the page is empty because of some js errors.
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.
The same errors are persistent on the main branch. I will create an issue for this.
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 to test changes for map of science in Nemo themes if that didn't work and still doesn't work?
Maybe it is worth to try to fix it?
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 syntax error (a single redundant bracket was misplaced almost a decade ago 😄) Should I leave this fix here or create a separate PR and issue since this fix is oneliner?
VIVO GitHub issue: 3931
Linked Vitro PR
What does this pull request do?
This pull request focuses on the removal of the Qtip library from the project, substituting it with Floating ui Library. The design has been configured to maintain similarity with the previous implementation using Qtip.
Only the Vitro part of PR is mandatory for the popper to work, other changes in Vitro and VIVO PR are only to initiate tooltip and style it.
What's new?
The Popper tooltip automatically identifies available screen space for display.
Before:
After:
How should this be tested?
On the Individual profile page:
On the Search results page:
On the MapOfScience page:
Additional Notes:
Interested parties
@VIVO-project/vivo-committers