Bug 1804638: add tooltip to devconsole monitoring graph #4363
Conversation
openshift-ci-robot
added
the
size/S
|
/retitle Bug 1804638: add tooltip to devconsole monitoring graph |
openshift-ci-robot
changed the title
|
@vikram-raj: This pull request references Bugzilla bug 1804638, which is invalid:
Comment In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
openshift-ci-robot
added
the
bugzilla/invalid-bug
|
/bugzilla refresh |
openshift-ci-robot
added
the
bugzilla/valid-bug
|
@vikram-raj: This pull request references Bugzilla bug 1804638, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
openshift-ci-robot
removed
the
bugzilla/invalid-bug
|
/kind bug |
openshift-ci-robot
added
the
kind/bug
| patchQuery: (v: QueryObj) => dispatch(queryBrowserPatchQuery(0, v)), | ||
| }); | ||
|
|
||
| export default connect(null, mapDispatchToProps)(MonitoringDashboardGraph); |
There was a problem hiding this comment.
can we have type here as well?
openshift-ci-robot
added
size/M
vikram-raj
force-pushed
the
odc-3099
branch
2 times, most recently
from
ecf3664
to
9cc357a
Compare
| @@ -43,4 +55,11 @@ const MonitoringDashboardGraph: React.FC<MonitoringDashboardGraphProps> = ({ | |||
| ); | |||
| }; | |||
|
|
|||
| export default MonitoringDashboardGraph; | |||
| const mapDispatchToProps = (dispatch): DispatchProps => ({ | |||
| patchQuery: (v: QueryObj) => dispatch(queryBrowserPatchQuery(0, v)), | |||
There was a problem hiding this comment.
This seems very odd to me that every graph is using the same index.
@kyoto can you please provide insight into the proper usage.
There was a problem hiding this comment.
Always using the index 0 looks wrong to me, so I'm not sure why this appears to be working.
For the admin perspective, I used onMouseEnter as a workaround for the problem (see #4060).
The root problem is that the tooltips assume the query is in Redux, which is actually only true for the Metrics page. Longer term, we should aim to fix that in the tooltips and remove these workarounds.
There was a problem hiding this comment.
Thanks @kyoto
Longer term we will move to share more of the dashboards.
|
/lgtm verified locally, seems to work as expected |
| @@ -2,7 +2,7 @@ import * as React from 'react'; | |||
| import * as _ from 'lodash'; | |||
| import { requirePrometheus } from '@console/internal/components/graphs'; | |||
| import { topWorkloadMetricsQueries } from '../queries'; | |||
| import MonitoringDashboardGraph from '../dashboard/MonitoringDashboardGraph'; | |||
| import ConnectedMonitoringDashboardGraph from '../dashboard/MonitoringDashboardGraph'; | |||
There was a problem hiding this comment.
There's no need to change all these from MonitoringDashboardGraph to ConnectedMonitoringDashboardGraph. Infact you shouldn't care. You should use the default import as is.
I suggest changing all these import name changes back to simply MonitoringDashboardGraph
There was a problem hiding this comment.
Ok so you'll need to change the name because of the eslint rule for using the same name as an exported value.
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: christianvogt, invincibleJai, vikram-raj 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 |
openshift-ci-robot
added
the
approved
|
@vikram-raj: All pull requests linked via external trackers have merged. Bugzilla bug 1804638 has been moved to the MODIFIED state. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
/cherry-pick release-4.4 |
|
@vikram-raj: new pull request created: #4455 In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
Fixes:
https://issues.redhat.com/browse/ODC-3099
Analysis / Root cause:
Redux were not get updated with query. So that tooltip component is not able get the query and label.
Solution Description:
Added query to redux.
Screen shots / Gifs for design review:
Browser conformance: