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

Use a tooltips library #3002

Merged
merged 25 commits into from
Feb 22, 2023
Merged

Use a tooltips library #3002

merged 25 commits into from
Feb 22, 2023

Conversation

maxpatiiuk
Copy link
Member

@maxpatiiuk maxpatiiuk commented Feb 16, 2023

To test:

  • Different places that use tooltips
    • Make sure tooltip is appropriate and useful
    • Make sure tooltip does not overflow the screen
  • Tooltips appear after an appropriate amount of time (i.e, after a duration that makes sense)
  • Tooltip text can be selected
  • Tooltips can be activated with keyboard
  • Tooltips can be used on a touchscreen device (mobile phone)
  • Disabled elements fallback to browser default tooltips
  • Tooltips look nice in dark mode and light mode
  • Tooltips have sufficient contrast from the background elements to be easily visible and readable in all places. if this is not good enough, we can consider changing background color or adding some shadow in light mode
  • Custom tooltips can be disabled in user preferences (Main -> UI -> Moder tooltips)

Some bugs found so far:

Overlaps with content:
Screenshot 2023-02-16 at 00 11 34

Causes page overflow:
Screenshot 2023-02-16 at 00 11 44

Without contexts

Without having to rewrite all the usages

Just listens for hover on elements with "title" attribute and uses that
title as a tooltip text

Thus, you don't have to know how to use our tooltip library to use it.
A pit of success!
@maxpatiiuk maxpatiiuk requested review from a team and CarolineDenis February 16, 2023 06:20
Triggered by 177fe66 on branch refs/heads/tooltips
@grantfitzsimmons
Copy link
Member

grantfitzsimmons commented Feb 16, 2023

Screen.Recording.2023-02-16.at.6.53.20.AM.mov

I'm not sure that having the tooltip jump from one item to another between hovering. I don't think it should be jumping around if possible.

It looks great on the side navigation menu but it should only shift if the items are within 2-3 items of each other. I know that might be complicated to implement, so it would be worth changing shifting behavior if it is difficult.

@grantfitzsimmons
Copy link
Member

image

Tooltips are still visible and at the forefront when a dialog is opened. Click on Reports and see the behavior

Copy link
Member

@grantfitzsimmons grantfitzsimmons left a comment

Choose a reason for hiding this comment

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

There are a number of usability concerns I have about the current implementation– many of which may be resolved by making the hover requirement a longer period of time before displaying the tooltips.

The navigation menu items should have the tooltip display at the current speed, but not every button or field in my opinion.


Another issue– if you have a tooltip open and the dialog changes or disappears the tooltip remains: See "Edit" in this instance

Screen.Recording.2023-02-16.at.7.05.25.AM.mov

@grantfitzsimmons
Copy link
Member

grantfitzsimmons commented Feb 16, 2023

Causes page overflow: Screenshot 2023-02-16 at 00 11 44

Can we not have tooltips be many more lines long and have it wrap? Set a max width for each tooltip?

@maxpatiiuk
Copy link
Member Author

Can we not have tooltips be many more lines long and have it wrap? Set a max width for each tooltip?

We can, but those fixed would have to be applied on case-by case basis (if translation is longer in some language, the UI breaks; if tooltip is near the edge of the screen, the max width has to be small).

Instead, I enabled a special feature in the tooltip library that would auto shift the tooltip to fit on the screen:

image

@maxpatiiuk
Copy link
Member Author

maxpatiiuk commented Feb 17, 2023

I'm not sure that having the tooltip jump from one item to another between hovering. I don't think it should be jumping around if possible.

It's a bug. Caused by animations being enabled (we have a global style saying all transitions should be animated - most of the time it's good - exceptions are the dialog drag and the tooltip move).

I had animations off so didn't detect this. Should be fixed now.

There are a number of usability concerns I have about the current implementation– many of which may be resolved by making the hover requirement a longer period of time before displaying the tooltips.

@grantfitzsimmons though, there is another question. I made tooltips have an 800ms delay before being shown for the first time, but then if you move your mouse to another element, the tooltip for the new element is displayed right away (assuming you moved you mouse to new element within the 1000ms threshold). Should I change that behavior to require 800ms for each hover? or maybe some smaller number?

For reference, I added an exception for the the side menu tooltips so they don't have any delay

Or would you like me to temporarily add user prefs for customizing these timeout durations so that we can fine-tune them and set good defaults?

@maxpatiiuk
Copy link
Member Author

maxpatiiuk commented Feb 17, 2023

Tooltips are still visible and at the forefront when a dialog is opened. Click on Reports and see the behavior

Fixed. Tooltip is now automatically closed if you click on the element that triggered the tooltip (i.e, clicked on the "Edit" button), or the element was removed (you clicked the "ESC" key to close the dialog while hovering over the "Edit" button)

Triggered by 45534d2 on branch refs/heads/tooltips
@grantfitzsimmons
Copy link
Member

image

This is with a VERY long description

image

Maybe we need to add a shadow or outline to these tooltips

Put tooltip at the bottom always, unless manually said otherwise or
there is not enough space

Fixes #3009
@maxpatiiuk
Copy link
Member Author

Fixes #3009

@maxpatiiuk
Copy link
Member Author

Maybe we need to add a shadow or outline to these tooltips

Added a shadow

@maxpatiiuk
Copy link
Member Author

maxpatiiuk commented Feb 22, 2023

@grantfitzsimmons If this is good now, we can merge it and release

Copy link
Member

@grantfitzsimmons grantfitzsimmons left a comment

Choose a reason for hiding this comment

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

Ok– you have won me over. There is more work to do here, so don't think this is the last time we will see tooltip tomfoolery

@grantfitzsimmons
Copy link
Member

grantfitzsimmons commented Feb 22, 2023

@maxpatiiuk Just need to resolve the conflict & we can release

@maxpatiiuk maxpatiiuk merged commit 3f856e1 into production Feb 22, 2023
@maxpatiiuk maxpatiiuk deleted the tooltips branch February 22, 2023 18:26
@specifysoftware
Copy link

This pull request has been mentioned on Specify Community Forum. There might be relevant details there:

https://discourse.specifysoftware.org/t/specify-7-8-6-release-announcement/1064/1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants