search

concourse / concourse

Concourse is a container-based continuous thing-doer written in Go.
https://concourse-ci.org
Apache License 2.0
7.35k stars 846 forks source link

tooltip for "new version" in build view goes under topbar #3921

Closed mockersf closed 4 years ago

mockersf commented 5 years ago

Bug Report

When the first line of a build in the view is a new resource, the tooltip goes under the topbar. Same for tooltip with durations

Steps to Reproduce

Go to a build with a new version of a resources that is used as the first step of a job. Mouse over the yellow arrow

Expected Results

I can read the tooltip

Actual Results

The tooltip is hidden under the topbar

Additional Context

tooltip-topbar

It looks like it's a CSS problem with the overflow behaviour that has changed with the addition of the sidebar, but I'm not sure how to fix it

Version Info

jomsie commented 5 years ago

Maybe this could fit into a slightly broader 'smart tooltip' epic together with #3813

jamieklassen commented 4 years ago

Since #4206 was reverted, this issue persists.

jamieklassen commented 4 years ago

I noticed today, that updateTooltip is exposed by StepTree but only called in Build (to handle the hover and clock tick cases): https://github.com/concourse/concourse/blob/master/web/elm/src/Build/Build.elm#L329 https://github.com/concourse/concourse/blob/master/web/elm/src/Build/Build.elm#L425

I think we should refactor things so that the .hoverCounter field lives closer to StepTree, where it is accessed. Hopefully then we can make use of the Tooltip.handleCallback a little more seamlessly.

jamieklassen commented 4 years ago

We got to the point where hovering a first-occurrence icon would trigger a GetViewportOf effect, but it took us a while to understand why a corresponding GotViewport callback was not being received. In some sense the culprit was that the function toHtmlID was causing an empty string to be passed to Browser.Dom.getViewportOf, and also that the first-occurrence icon did not have an ID to be passed. Ideally there would be some way to extract this cross-cutting concern without the need for exhaustive unit tests (i.e. there should be some kind of parametrized type for tooltippable things so that there is no need to separately assert that every tooltippable element has the correct id).

Frankly, it might not be possible to achieve this goal without extracting a more transparent type that will act as an alternative to Html, which could be a pretty broad-ranging change. Let's still keep this desire in mind, but avoid wandering down any garden paths.

jamieklassen commented 4 years ago

The plan right now is to move the tooltip outside the scrollable-body container, so that it is not affected by overflow attributes. @matthewpereira wisely observed that this approach might impact accessibility in some way, and I'm just writing it down here in case I forget.