Skip to content
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 Bootstrap Tooltip #3937

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

milospp
Copy link
Contributor

@milospp milospp commented Jan 30, 2024

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 Bootstrap's tooltip that utilizes Popper 2 as its underlying mechanism. The design has been configured to maintain similarity with the previous implementation using Qtip.

What's new?

The Bootstrap tooltip automatically identifies available screen space for display but is limited to top, bottom, left, and right orientations, excluding corners.

Before:

image image image image

After:

Individual Search resultd download MapOfScience

How should this be tested?

On the Individual profile page:

  • Hover, Research area icon image (#researchAreaIcon)
  • Click, Contanct Info link icon image (#uriIcon)

On the Search results page:

  • Click, download icon image (#downloadIcon)

On the MapOfScience page:

  • Hover over next elements, #mageIconOne, #exploreInfoIcon, #compareInfoIcon, #imageIconThree

Additional Notes:

  • This pull request may impact dependencies, as it involves the removal of the Qtip library and the addition of Popper. Please be mindful of potential issues in this regard.

Interested parties

@VIVO-project/vivo-committers

@chenejac chenejac linked an issue Feb 6, 2024 that may be closed by this pull request
Copy link
Member

@gneissone gneissone left a 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.

@milospp
Copy link
Contributor Author

milospp commented Feb 12, 2024

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.

Completely overlooked testing on other themes since the decision to prioritize Wilma and phase out the others in the future.
I've now fixed it for all other themes (Tenderfoot, Nemo, Vitro) and included Bootstrap 5 for tooltips in those themes, which might cause issues if any Bootstrap 3 code was overlooked. If you spot anything else, just give me a heads-up so I can fix it. Thanks!

Copy link
Collaborator

@litvinovg litvinovg left a 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)?

/* TOOLTIP ----------------------------------------> */
/* -------------------------------------------------> */
.vivoTooltip {
opacity: 1 !important;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think !important is the last resort and should be avoided everywhere if possible.

/* -------------------------------------------------> */
/* TOOLTIP ----------------------------------------> */
/* -------------------------------------------------> */
.vivoTooltip {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would try to avoid style duplication, maybe it would be better to define styles once and include in all themes.

},{
querySelector: "#exploreInfoIcon",
data: {
title: "<div style='padding: 16px 22px; max-width: 400px;'>" + $('#exploreTooltipText').html() + "</div>",
Copy link
Collaborator

Choose a reason for hiding this comment

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

The same in-lined styles defined multiple times. It would be better avoid inline styles, instead define in a css file

@chenejac chenejac marked this pull request as draft June 5, 2024 06:37
@chenejac chenejac added this to the v1.16 milestone Jun 5, 2024
@chenejac chenejac removed this from the v1.16 milestone Oct 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Replace qtip dependency
4 participants