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

dashboard: Allow multi-line charts in UtilizationItem #4008

Merged
merged 1 commit into from Jan 23, 2020
Merged

dashboard: Allow multi-line charts in UtilizationItem #4008

merged 1 commit into from Jan 23, 2020

Conversation

rawagner
Copy link
Contributor

@rawagner rawagner commented Jan 20, 2020 •
edited

replaces #3089

@mareklibra @andybraren please take a look

Screenshot from 2020-01-20 14-09-38
Screenshot from 2020-01-20 14-09-17

note that the cursor is always in the middle of tooltip - the behavior is from VictoryVoronoiContainer which I was not able to alter
Screenshot from 2020-01-20 14-27-36

@openshift-ci-robot openshift-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Jan 20, 2020
@openshift-ci-robot openshift-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. component/core Related to console core functionality component/dashboard Related to dashboard component/kubevirt Related to kubevirt-plugin component/metal3 Related to metal3-plugin component/shared Related to console-shared labels Jan 20, 2020
@andybraren
Copy link
Contributor

image

note that the cursor is always in the middle of tooltip - the behavior is from VictoryVoronoiContainer which I was not able to alter

Looks a little odd 😄

@dlabrecq would you happen to know why this is happening? The Victory example and PatternFly examples don't seem to have this issue. Is the triangle being missing related maybe?

@rawagner
Copy link
Contributor Author

image

note that the cursor is always in the middle of tooltip - the behavior is from VictoryVoronoiContainer which I was not able to alter

Looks a little odd

@dlabrecq would you happen to know why this is happening? The Victory example and PatternFly examples don't seem to have this issue. Is the triangle being missing related maybe?

it is actually the same as Victory example where you have two lines and one tooltip for both - see https://formidable.com/open-source/victory/docs/victory-voronoi-container/#mousefollowtooltips

@andybraren
Copy link
Contributor

Ah I see. Would we be able to use either the behavior of voronoiBlacklist (appearing over the top-most data points) or voronoiDimension's behavior as a backup option? The mouseFollowTooltips behavior is very odd. 🙂

@rawagner
Copy link
Contributor Author

rawagner commented Jan 21, 2020 •
edited

mouseFollowTooltips is not set, but the points are so close together that it looks like it is (correct link would be https://formidable.com/open-source/victory/docs/victory-voronoi-container/#voronoidimension)
voronoiDimension is already set to x
voronoiBlacklist could work but I need to choose a line which to block and that line can be on top and then a few minutes later below the other one.

I used centerOffset to not have cursor in middle of tooltip. WDYT ?

Screenshot from 2020-01-21 13-06-57

@rawagner
Copy link
Contributor Author

constrainToVisibleArea

that is not set. The tooltip is actually able to go outside the chart.

andybraren
andybraren approved these changes Jan 22, 2020
View reviewed changes
Copy link
Contributor

@andybraren andybraren left a comment

Choose a reason for hiding this comment

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

I used centerOffset to not have cursor in middle of tooltip. WDYT ?

Much better, thank you! LGTM

@mareklibra mareklibra mentioned this pull request Jan 22, 2020
Closed
1 task
mareklibra
mareklibra reviewed Jan 22, 2020
View reviewed changes
...end/packages/console-shared/src/components/dashboard/utilization-card/TopConsumerPopover.tsx Outdated
@@ -21,6 +21,7 @@ import { getName, getNamespace } from '../../..';
import { DashboardCardPopupLink } from '../dashboard-card/DashboardCardLink';

import './top-consumer-popover.scss';
import { getPrometheusQueryResponse } from '@console/internal/actions/dashboards';
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: please move this line up

...tend/packages/kubevirt-plugin/src/components/dashboards-page/vm-dashboard/vm-utilization.tsx Outdated
import {
useMetricDuration,
Duration,
} from '@console/shared/src/components/dashboard/duration-hook';
import { VMDashboardContext } from '../../vms/vm-dashboard-context';
import { findVMPod } from '../../../selectors/pod/selectors';
import { getUtilizationQueries, VMQueries } from './queries';
import { getUtilizationQueries, getMultilineUtilizationQueries, VMQueries } from './queries';
import { ByteDataTypes } from '@console/shared/src/graph-helper/data-utils';
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: likewise

frontend/public/components/graphs/area.tsx Outdated
@@ -1,4 +1,5 @@
import * as React from 'react';
import * as _ from 'lodash-es';
Copy link
Contributor

Choose a reason for hiding this comment

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

just lodash for new code

@mareklibra
Copy link
Contributor

Nitpicks only, otherwise lgtm

@rawagner
Copy link
Contributor Author

@mareklibra fixed

@mareklibra
Copy link
Contributor

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 23, 2020
@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andybraren, mareklibra, rawagner

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@jelkosz
Copy link

jelkosz commented Jan 23, 2020

/test e2e-gcp-console

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-merge-robot openshift-merge-robot merged commit b78c4fc into openshift:master Jan 23, 2020
@spadgett spadgett added this to the v4.4 milestone Jan 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mareklibra mareklibra mareklibra left review comments

@andybraren andybraren andybraren approved these changes

@dtaylor113 dtaylor113 Awaiting requested review from dtaylor113

Assignees

@mareklibra mareklibra

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. component/core Related to console core functionality component/dashboard Related to dashboard component/kubevirt Related to kubevirt-plugin component/metal3 Related to metal3-plugin component/shared Related to console-shared lgtm Indicates that a PR is ready to be merged. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Milestone
v4.4
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@rawagner @andybraren @mareklibra @openshift-ci-robot @jelkosz @openshift-bot @spadgett @openshift-merge-robot