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.

Sign up for GitHub

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

Jump to bottom

Replace Qtip Library with Floating ui #439

Open
wants to merge 26 commits into
base: main
Choose a base branch
from
Open

Replace Qtip Library with Floating ui #439

wants to merge 26 commits into from

Conversation

milospp
Copy link
Contributor

@milospp milospp commented Jan 30, 2024 •
edited
Loading

VIVO GitHub issue: 3931
Linked VIVO 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:

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

@milospp milospp mentioned this pull request Jan 30, 2024
Open
@chenejac chenejac requested a review from gneissone January 31, 2024 09:02
@chenejac chenejac linked an issue Feb 6, 2024 that may be closed by this pull request
Replace qtip dependency vivo-project/VIVO#3931
Open
gneissone
gneissone requested changes Feb 7, 2024
View reviewed changes
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.

Please see review vivo-project/VIVO#3937 (review) for general remarks.

These changes work well with Wilma, but do not work with the default vitro template since vitro does not include bootstrap. Wonder if the tooltips can just be dropped completely in the vitro template, since it was intended to be minimal in the first place...

@chenejac chenejac requested a review from gneissone February 13, 2024 09:32
@chenejac chenejac marked this pull request as draft April 12, 2024 11:05
@milospp milospp marked this pull request as ready for review November 20, 2024 00:30
@milospp milospp force-pushed the fix-tooltip branch 2 times, most recently from 51646ef to ddddae7 Compare December 8, 2024 22:21
@milospp milospp changed the title Replace Qtip Library with Bootstrap Tooltip Replace Qtip Library with Popper Dec 10, 2024
@milospp milospp changed the title Replace Qtip Library with Popper Replace Qtip Library with Floating ui Dec 11, 2024
@chenejac chenejac requested a review from litvinovg December 23, 2024 09:39
litvinovg
litvinovg reviewed Jan 9, 2025
View reviewed changes
webapp/src/main/webapp/themes/vitro/css/vitroTheme.css
@@ -1473,3 +1473,8 @@ form.customForm #requiredLegend {
font-style: italic;
margin-top: .5em;
}

.show {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you tell me where this css class is used?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When I tried to upgrade to Bootstrap 5 this was a fix to handle the tabs section. Bootstrap automatically adds "show" class to the tab of the panel that needs to show.
Since the bootstrap 5 upgrade is reverted from this PR, this is actually useless and I will remove it.

@litvinovg
Copy link
Contributor

Doesn't work on Vitro for some reason.
Screenshot at 2025-01-09 15-43-10

@milospp
Copy link
Contributor Author

milospp commented Jan 23, 2025

Doesn't work on Vitro for some reason. Screenshot at 2025-01-09 15-43-10

I couldn't reproduce the same bug, can you please provide more information on when this bug happened?

@litvinovg
Copy link
Contributor

Doesn't work on Vitro for some reason. Screenshot at 2025-01-09 15-43-10

I couldn't reproduce the same bug, can you please provide more information on when this bug happened?

Sure. Just build Vitro, create any individual, open page profile and notice error in console.

@milospp
Copy link
Contributor Author

milospp commented Jan 24, 2025

Doesn't work on Vitro for some reason. Screenshot at 2025-01-09 15-43-10

I couldn't reproduce the same bug, can you please provide more information on when this bug happened?

Sure. Just build Vitro, create any individual, open page profile and notice error in console.

https://youtu.be/X56Hxjf6seI
This is what I got, ignore those two errors, it is related to the LastPass Chrome extension.
Can you confirm whether I reproduced the steps correctly? If yes can you please try again but try to hard reload the page (Ctrl + F5 in Chrome) after building

@litvinovg
Copy link
Contributor

Doesn't work on Vitro for some reason. Screenshot at 2025-01-09 15-43-10

I couldn't reproduce the same bug, can you please provide more information on when this bug happened?

Sure. Just build Vitro, create any individual, open page profile and notice error in console.

https://youtu.be/X56Hxjf6seI This is what I got, ignore those two errors, it is related to the LastPass Chrome extension. Can you confirm whether I reproduced the steps correctly? If yes can you please try again but try to hard reload the page (Ctrl + F5 in Chrome) after building

First step is to build Vitro, not VIVO.

@litvinovg
Copy link
Contributor

Doesn't work on Vitro for some reason. Screenshot at 2025-01-09 15-43-10

I couldn't reproduce the same bug, can you please provide more information on when this bug happened?

Sure. Just build Vitro, create any individual, open page profile and notice error in console.

https://youtu.be/X56Hxjf6seI This is what I got, ignore those two errors, it is related to the LastPass Chrome extension. Can you confirm whether I reproduced the steps correctly? If yes can you please try again but try to hard reload the page (Ctrl + F5 in Chrome) after building

First step is to build Vitro, not VIVO.

@milospp Did you manage to reproduce the issue?
You were building and deploying VIVO with Vitro theme, instead build and install Vitro application.

@milospp
Copy link
Contributor Author

milospp commented Jan 28, 2025

d deploying VIVO with Vitro theme, instead build and install Vitro application.

Yes, I reproduced it. I just didn't work on it until now. I updated the code. Can you check it out now?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@litvinovg litvinovg litvinovg left review comments

@gneissone gneissone Awaiting requested review from gneissone

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Replace qtip dependency
3 participants
@milospp @litvinovg @gneissone