Open Bug 1581497 Opened Last month Updated Yesterday

The graph-tooltip is unaligned with its datapoint

Categories

(Tree Management :: Perfherder, defect, P3)

defect

Tracking

(Not tracked)

ASSIGNED

People

(Reporter: alexandrui, Assigned: sclements, Mentored)

References

Details

Attachments

(3 files, 1 obsolete file)

Attached image Screenshot from 2019-09-16 13-08-48.png (obsolete) —

The graph-tooltip is unaligned with its datapoint and it overflows the VictoryContainer for the right-most datapoints.

Attachment #9092980 - Attachment is obsolete: true

Hrmm, it only does this when it's in responsive mode (a recent change I made with the last major pr). I'm really not sure why it does that and it might require a read up on svg's in order to come up with a good solution. Another option would be to use the className prop on the VictoryBrushContainer and VictoryZoomContainer and change it back to response={false} for both containers. You can then add media queries in the perf stylesheet to change the fixed width based on the viewport. I think this might be the easiest solution - the tooltips are positioned as expected this way and will prevent the graphs from wrapping onto the next row (with scroll bars when the viewport gets smaller).

As an example:

@media only screen and (min-width: 1200px) {
  .graph-container svg {
    max-width: 900px;
  }
  .graph-container {
    width: 900px !important;
  }
}

@media only screen and (min-width: 1700px) {
  .graph-container svg {
    max-width: 1350px;
  }
  .graph-container {
    width: 1350px !important;
  }
}

I'd go as far as supporting laptop size and you can add additional breaks/customize it as desired. You can see how bootstrap supports it here: https://getbootstrap.com/docs/4.3/layout/overview/

Thank you, Sarah! I will try what you suggested!

I made a PR. I am still having hard times with grouping them in separate commits after receiving a review. Anyway, the issue here is that the tooltip position is calculated for the total width of the page elements smaller than window.display.availWidth. When it overlaps, the formula doesn't fit anymore. What I did is to preserve the minimum width of the graph so it becomes responsive. I still don't like the center alignment of the legend when the graph is at the bottom of it.
I tried to find a formula to calculate the tooltip position for this situation so I am not forced to move it to the bottom, but I couldn't find yet.

Flags: needinfo?(sclements)

Yeah, I'm not really seeing a difference with the tool tip positioning when manually zooming the page (CMD +) in your latest pr, which I think is what you're trying to fix? Are you zooming the page because you want the text or certain elements to be bigger?

As for grouping commits separately - it looks like you did it OK. Next time just rename it differently like 'changes based on feedback'. I use git rebase -i to squash or rename commits but try to preserve the initial commit.

Flags: needinfo?(sclements)

I think I was trying the wrong way with that zoom. Actually, I have a big monitor and this is translated in higher resolution. At 1600*900 and 100% zoom my tooltip is first next resolution that the tooltip is unaligned. The are many variables here, including that the text is getting bigger with the zoom %, this is making the total width of the page elements to overflow the window.display.availWidth, which makes the tooltip to be unaligned. I am not sure I understood your first comment so I am reading over and over again to make sure I am getting it right.

I finally managed to apply your recommandations. I still don't like it because the graph is actually moved the graph under the legend and the legend alignment is centered. If we have several tests it will move the graph lower, which is now an UXish way at all. I am thinking to align horizontally the legend in the case the graph moves under it. Is it doable? Yeah, I know that it is getting more complicated, but I really don't like this result.

Flags: needinfo?(sclements)

Yes, we want to avoid having it look like that. The CSS code in comment 3 was meant as an example to the approach - not the exact dimensions you should set per view port. :) You'll need to adjust the width and maxWidth attributes so they look good for each view port breakpoint. You can also add as many breakpoints as needed to make the transition from one view port to another smooth (which I would recommend). Are you familiar with the responsive design mode feature in the dev tools panel? That will make this easier for you.

Flags: needinfo?(sclements)

Here is a short demo.

Flags: needinfo?(sclements)

The horizontal alignment looks OK but that seems like something that should happen at a much smaller view port - medium or small breakpoints: https://getbootstrap.com/docs/4.3/layout/overview/#responsive-breakpoints

Flags: needinfo?(sclements)

It should happen at a much smaller viewport, but looks like it happens also on my 22" (1920x1080) monitor, at 1600x900 (0.83 from 1920x1080). There's something I'm missing here, but I am not sure what...

Flags: needinfo?(sclements)
Flags: needinfo?(karlt)
Flags: needinfo?(cdawson)

Alexandru, you don't need to pull Karl and Cameron into this review since I'm already working with you. I think what might help is looking into how I've used the custom CSS class custom-col-xxl-auto. Reading up on bootstrap's use of flex would probably help too. This prevents the graph from wrapping onto the next row starting from the 1700px breakpoint. I find using the devtools inspector and manually making changes there when working with CSS (and to see what styles an element might be inheriting) to be helpful.

Flags: needinfo?(sclements)
Flags: needinfo?(karlt)
Flags: needinfo?(cdawson)
Flags: needinfo?(sclements)

I looked at the demo but the problem is that the graph sizes stay the same and horizontal scroll bars appear as the view port gets smaller. I don't think that's the right direction to go with this. Maybe we could go over some approaches/solutions I had in mind over video, if you're able to work a bit later (feel free to book something in my calendar - earliest 8am PST). If you need to move on to other projects, I could work on this when I have some time.

Flags: needinfo?(sclements)

I need to move on other projects as I have new priorities with the Q4 OKSs.
Thanks for your support, Sarah!

Assignee: alexandru.ionescu → sclements
Status: NEW → ASSIGNED
Priority: -- → P3
Duplicate of this bug: 1588812
You need to log in before you can comment on or make changes to this bug.