Skip to content

ToolTip overflow Fix and positioning#1054

Merged
BryonLewis merged 7 commits into
mainfrom
client/tooltip-fixes
Dec 1, 2021
Merged

ToolTip overflow Fix and positioning#1054
BryonLewis merged 7 commits into
mainfrom
client/tooltip-fixes

Conversation

@BryonLewis
Copy link
Copy Markdown
Collaborator

fixes #1051

  • Fixes the tooltip text overflow issue by allowing it to take the full size required.
  • Moves the tooltip to the most inside box upper right hand side
  • Sorts the items in the tooltip for inside-out.

@subdavis
Copy link
Copy Markdown
Contributor

subdavis commented Nov 17, 2021

Question 1

What's so bad about tooltip that follows mouse? It seems pretty responsive to me.

Screencast.2021-11-17.13.49.09.mp4

Question 2

Seems like this still isn't optimal. You can end up completely covering a bounding box with a tooltip with no way around it.

Screencast.2021-11-17.13.13.42.mp4

Question 3

Also seems like there's a race condition somewhere. The tooltip doesn't update sometimes when you move around.

Screencast.2021-11-17.13.12.16.mp4

@BryonLewis
Copy link
Copy Markdown
Collaborator Author

Question 1

What's so bad about tooltip that follows mouse? It seems pretty responsive to me.

If your happy with the responsiveness we can go with that. The only counterpoint is that we have had some complaints about the tooltip drawing icons and people not liking them following the mouse closely. Maybe instead of a hover based tool it requires a different action to trigger?

Question 2

Seems like this still isn't optimal. You can end up completely covering a bounding box with a tooltip with no way around it.
Yeah, this is probably a problem given that it appears in the same location for the inside box each time so it will always hide outside box.

Question 3

Also seems like there's a race condition somewhere. The tooltip doesn't update sometimes when you move around.

I'm having trouble consistently replicating this but I did manage to get it to happen. It seems to happen a bit more when zoomed in vs the entire image.

Thank you for the feedback and questions. I'm leaning more towards just having it attached to the mouse now.

@subdavis
Copy link
Copy Markdown
Contributor

With option 1 (follow pointer) I did notice that depending on zoom level, the tooltip can be under the pointer and intercept mouse interactions. IDK if there's a reliable way around that. Because the coordinates are in image-space and not screen-space, having a constant offset is probably undesirable.

@subdavis
Copy link
Copy Markdown
Contributor

If your happy with the responsiveness we can go with that. The only counterpoint is that we have had some complaints about the tooltip drawing icons and people not liking them following the mouse closely

As I recall, that was related to doing annotation. Someone thought the icons were in the way while drawing, which is fair. This you can turn off and isn't really useful while drawing anyway, so it's probably fine, but we should get someone like Matt or Roddy to actually decide.

@waxlamp
Copy link
Copy Markdown
Member

waxlamp commented Nov 17, 2021

With option 1 (follow pointer) I did notice that depending on zoom level, the tooltip can be under the pointer and intercept mouse interactions. IDK if there's a reliable way around that. Because the coordinates are in image-space and not screen-space, having a constant offset is probably undesirable.

I also like Option 1. As for the image space / screen space issue, my gut tells me to ask David Manthey about it. GeoJS has ways to plot things in screen space, if I am not mistaken.

@BryonLewis
Copy link
Copy Markdown
Collaborator Author

I updated it for positioning the tooltip in a standard position based on zoomLevel and mouse following. There is a bit of a lag in the positioning though

mousehover.mp4

I'm open to any alternatives?

@subdavis
Copy link
Copy Markdown
Contributor

This is perfect. I couldn't figure out from the code how you got the padding in screen space though, just curious

@BryonLewis
Copy link
Copy Markdown
Collaborator Author

This is perfect. I couldn't figure out from the code how you got the padding in screen space though, just curious

I just pushed the changes, I need to self review it. I'm still using the position function provided geoJS but I looked at what it does internally inside of geoJS, converted to display corrdinates, added the offset and then converted back into the geojs coordinates used for the display.

The zooming event only supplies info in screenspace (relative to the element) so I would have to do some more conversions to get it right and that coordinate doesn't change after each zoom. It was much easier to just save the last mousePosition and use it like a mouseMove function for the zooming instead.

@BryonLewis BryonLewis marked this pull request as ready for review December 1, 2021 15:37
subdavis
subdavis previously approved these changes Dec 1, 2021
Copy link
Copy Markdown
Contributor

@subdavis subdavis left a comment

Choose a reason for hiding this comment

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

Perfect. I tested pretty extensively including annotation and continuous mode, editing, deleteing, etc, all looking great.

Comment thread client/src/components/LayerManager.vue Outdated
}
});
hoverOvered.value = hoveredVals.sort((a, b) => a.maxX - b.maxX);
uiLayer.setToolTipWidget('customToolTip', (found.length > 0));
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should this technically be hoveredValues.length > 0 since there's a possibility to filter everything?

@BryonLewis BryonLewis merged commit 2fb75d1 into main Dec 1, 2021
@BryonLewis BryonLewis deleted the client/tooltip-fixes branch December 1, 2021 18:24
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.

[BUG] Fix Max width Tooltip display

3 participants