Skip to content
This repository has been archived by the owner on Apr 11, 2023. It is now read-only.

KIALI-2129 UXD: Move the grafana link to the topbar #876

Merged
merged 4 commits into from
Dec 11, 2018

Conversation

aljesusg
Copy link
Contributor

** Describe the change **

  • Move the Grafana Link to the topbar
  • Add the grafana Link to Workload Metrics
  • Move displaying metrics last x minutes to the toolbarRight

** Issue reference **

JIRA: KIALI-2129

** Screenshot **

Services inbound metrics

screenshot from 2018-12-11 11-18-18

Workload outbound metrics

workload_out

Workload inbound metrics

workload_in

@aljesusg aljesusg requested a review from jotak December 11, 2018 11:18
@@ -4,7 +4,8 @@ import { RouteComponentProps, withRouter } from 'react-router';
import Metrics, { MetricsProps } from '../components/Metrics/Metrics';

const mapStateToProps = (state: KialiAppState) => ({
isPageVisible: state.globalState.isPageVisible
isPageVisible: state.globalState.isPageVisible,
grafanaInfo: state.grafanaInfo
});
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added here the link for workloads @jotak

@abonas
Copy link
Contributor

abonas commented Dec 11, 2018

from UX perspective looking per the design. cc @cshinn

@aljesusg
Copy link
Contributor Author

Thanks @abonas I forgot mention @cshinn I added the PR in the design document.

@aljesusg aljesusg requested a review from cshinn December 11, 2018 12:35
@aljesusg aljesusg added the ux review required UX review is required label Dec 11, 2018
@jotak
Copy link
Contributor

jotak commented Dec 11, 2018

@aljesusg I'm not sure if it's related to your PR, but when I'm entering in metrics page I can see that the grafana info is being fetched twice. Hence if I'm having incorrect settings for Grafana I see the error twice.

Copy link
Contributor

@jotak jotak left a comment

Choose a reason for hiding this comment

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

Actually something is missing: if we remove the "Refreshing" label, we must add "every" as a prefix in the combo options. I've check the design doc and it's what is mocked up.

@aljesusg
Copy link
Contributor Author

ok @jotak I was not sure about add the every label, about the fetch of the grafana info I think that we can change this since we have this information when the user login kiali, we don't need fetch again.

@aljesusg aljesusg added the do not merge A PR is not ready to merge label Dec 11, 2018
30000: '30 sec',
60000: '1 min',
300000: '5 min'
5000: 'Every 5 sec',
Copy link
Contributor

Choose a reason for hiding this comment

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

This makes the graph refresh duplicate "Every": it's now showing like "Every Every 15 sec" :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hahhaa I'll cahnge it ^^

@aljesusg aljesusg removed the do not merge A PR is not ready to merge label Dec 11, 2018
Copy link
Contributor

@jotak jotak left a comment

Choose a reason for hiding this comment

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

LGTM

@cfcosta
Copy link
Contributor

cfcosta commented Dec 11, 2018

@aljesusg do the testing for this again with the proper (built) Kiali UI, this might be affected by facebook/react#12856.

@cfcosta cfcosta merged commit cca2104 into kiali:master Dec 11, 2018
@aljesusg
Copy link
Contributor Author

@cfcosta we were waiting to @cshinn and @abonas to review it...please don't merge it if there is label ux-review

@cfcosta
Copy link
Contributor

cfcosta commented Dec 11, 2018

Oh, my bad, sorry about that.

@aljesusg aljesusg deleted the k_2129 branch September 9, 2019 08:36
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
ux review required UX review is required
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants