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.

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

NETOBSERV-867 UI: Table Histogram help #295

Merged
merged 4 commits into from
Mar 14, 2023
Merged

Conversation

jpinsonneau
Copy link
Contributor

@jpinsonneau jpinsonneau commented Feb 22, 2023

These changes brings a reusable guided tour and implements it for histogram.
It automatically shows up at first usage and be called again from the "?" button.

Screencast.from.2023-02-22.14-38-17.webm

Suggestions:

  • increase the image sizes
  • wording
  • restore tooltips if not currently in the tour

@jpinsonneau
Copy link
Contributor Author

@andrew-ronaldson what do you think about this approach ?

@@ -1291,6 +1295,7 @@ export const NetflowTraffic: React.FC<{
{slownessReason()}
</Alert>
)}
<GuidedTourPopover id="netobserv" ref={guidedTourRef} />
Copy link
Contributor Author

@jpinsonneau jpinsonneau Feb 22, 2023

Choose a reason for hiding this comment

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

The guided tour is available for any components as soon as you provide guidedTourHandle from guidedTourRef.current as props

Comment on lines 145 to 194
const [guidedTourDone, setGuidedTourDone] = useLocalStorage<boolean>(LOCAL_STORAGE_HISTOGRAM_GUIDED_TOUR_DONE_KEY);
React.useEffect(() => {
if (!guidedTourHandle) {
return;
}

guidedTourHandle.updateTourItems([
{
title: t('Histogram'),
description: t(
// eslint-disable-next-line max-len
'The following bar chart represent the number of logs across time. You can select a portion of it to drill down into the selected time range and filter the content below accordingly.'
),
assetName: 'histogram.gif',
minWidth: '500px',
ref: containerRef
},
{
title: t('Zoom buttons'),
description: zoomButtonTips(),
assetName: 'histogram-zoom.gif',
position: PopoverPosition.bottom,
ref: zoomRef
},
{
title: t('Arrow buttons'),
description: arrowButtonTips(),
ref: arrowRef
},
{
title: t('Page buttons'),
description: pageButtonTips(),
assetName: 'histogram-pages.gif',
minWidth: '500px',
position: PopoverPosition.bottom,
ref: pageRef
}
]);

if (!guidedTourDone) {
setGuidedTourDone(true);
guidedTourHandle.startTour();
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Every component using the guided tour can call updateTourItems to define a tour (and keep refs updated even if the view is removed from the dom) and startTour to open it from the begining using the guidedTourHandle provided as prop.

Copy link
Member

Choose a reason for hiding this comment

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

nice 👍

@Amoghrd
Copy link
Contributor

Amoghrd commented Feb 27, 2023

/ok-to-test

@openshift-ci openshift-ci bot added the ok-to-test To set manually when a PR is safe to test. Triggers image build on PR. label Feb 27, 2023
@github-actions
Copy link

New image: ["quay.io/netobserv/network-observability-console-plugin:c0900d0"]. It will expire after two weeks.

@Amoghrd
Copy link
Contributor

Amoghrd commented Mar 1, 2023

@jpinsonneau For the first and the last tip, the images are quite small to see what is happening. Other than that the tips look good!

@jpinsonneau
Copy link
Contributor Author

@jpinsonneau For the first and the last tip, the images are quite small to see what is happening. Other than that the tips look good!

Should I increase the size ? Or would it be better to use static images instead of animated gif ?

@Amoghrd
Copy link
Contributor

Amoghrd commented Mar 2, 2023

Increasing the size would be helpful

@jotak
Copy link
Member

jotak commented Mar 3, 2023

lgtm apart from the typo, and would defer to other folks for more reviews (also @andrew-ronaldson for UX )

@github-actions github-actions bot removed the ok-to-test To set manually when a PR is safe to test. Triggers image build on PR. label Mar 3, 2023
@jotak
Copy link
Member

jotak commented Mar 3, 2023

/lgtm

@openshift-ci openshift-ci bot added the lgtm label Mar 3, 2023
Copy link
Contributor

@skrthomas skrthomas left a comment

Choose a reason for hiding this comment

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

LGTM, one suggestion and a few articles that need to be added for completeness/clarity

web/locales/en/plugin__netobserv-plugin.json Outdated Show resolved Hide resolved
web/locales/en/plugin__netobserv-plugin.json Outdated Show resolved Hide resolved
web/locales/en/plugin__netobserv-plugin.json Outdated Show resolved Hide resolved
@openshift-ci openshift-ci bot removed the lgtm label Mar 13, 2023
@codecov
Copy link

codecov bot commented Mar 13, 2023

Codecov Report

Merging #295 (9cec886) into main (bc5b153) will decrease coverage by 0.94%.
The diff coverage is 39.74%.

@@            Coverage Diff             @@
##             main     #295      +/-   ##
==========================================
- Coverage   59.73%   58.79%   -0.94%     
==========================================
  Files         118      147      +29     
  Lines        4704     6456    +1752     
  Branches      759      769      +10     
==========================================
+ Hits         2810     3796     +986     
- Misses       1769     2447     +678     
- Partials      125      213      +88     
Flag Coverage Δ
uitests 59.48% <39.74%> (-0.25%) ⬇️
unittests 56.84% <ø> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
web/src/components/metrics/histogram.tsx 17.47% <16.66%> (-2.30%) ⬇️
web/src/components/guided-tour/guided-tour.tsx 51.11% <51.11%> (ø)
web/src/components/netflow-traffic.tsx 54.24% <100.00%> (+0.20%) ⬆️
web/src/utils/local-storage-hook.ts 67.79% <100.00%> (+0.55%) ⬆️

... and 28 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

jpinsonneau and others added 3 commits March 13, 2023 15:45
@Amoghrd
Copy link
Contributor

Amoghrd commented Mar 13, 2023

/ok-to-test

@openshift-ci openshift-ci bot added the ok-to-test To set manually when a PR is safe to test. Triggers image build on PR. label Mar 13, 2023
@github-actions
Copy link

New image: ["quay.io/netobserv/network-observability-console-plugin:e68d1e5"]. It will expire after two weeks.

@github-actions github-actions bot removed the ok-to-test To set manually when a PR is safe to test. Triggers image build on PR. label Mar 13, 2023
@jpinsonneau
Copy link
Contributor Author

@Amoghrd I've just finished to address all the feedbacks

/ok-to-test

@openshift-ci openshift-ci bot added the ok-to-test To set manually when a PR is safe to test. Triggers image build on PR. label Mar 13, 2023
@github-actions
Copy link

New image: ["quay.io/netobserv/network-observability-console-plugin:918b565"]. It will expire after two weeks.

@Amoghrd
Copy link
Contributor

Amoghrd commented Mar 13, 2023

/label qe-approved

@openshift-ci openshift-ci bot added the qe-approved QE has approved this pull request label Mar 13, 2023
@jotak
Copy link
Member

jotak commented Mar 14, 2023

/lgtm

@openshift-ci openshift-ci bot added the lgtm label Mar 14, 2023
@jpinsonneau
Copy link
Contributor Author

/approve

@openshift-ci
Copy link

openshift-ci bot commented Mar 14, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jpinsonneau

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

@jpinsonneau jpinsonneau merged commit a99daf8 into netobserv:main Mar 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved lgtm ok-to-test To set manually when a PR is safe to test. Triggers image build on PR. qe-approved QE has approved this pull request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants