-
Notifications
You must be signed in to change notification settings - Fork 15
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-789 UI: Table Histogram shortcuts #272
Conversation
Skipping CI for Draft Pull Request. |
/hold for #271 |
/ok-to-test |
New image: ["quay.io/netobserv/network-observability-console-plugin:7b71b24"]. It will expire after two weeks. |
/unhold |
@jotak @OlivierCazade I have merged #271 and rebased this PR so you can review these extra changes when you have some time @andrew-ronaldson feel free to give your feedback here or in a followup Thank you ! |
/ok-to-test |
/label ok-to-test |
/ok-to-test |
New image: ["quay.io/netobserv/network-observability-console-plugin:8e810b0"]. It will expire after two weeks. |
So @jpinsonneau The |
Well these values are not fixed; it's context related:
I'm not sure to get that point. Custom time range is fully free. You can pick any value there and the histogram shortcuts will do so. |
/label qe-approved |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, my comment was more a general comment
@@ -1005,6 +1007,70 @@ export const NetflowTraffic: React.FC<{ | |||
// eslint-disable-next-line react-hooks/exhaustive-deps | |||
}, [match, filters]); | |||
|
|||
const moveRange = React.useCallback( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
More a general comment here.
In my opinion, NetflowTraffic has become too big and we should split it for maintainability.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fully aligned with that. I'm planning to move the drawer away as I did in my PoC on connection tracking.
We can also move the modals away from that file creating a dedicated component.
I'm adding a note in https://issues.redhat.com/browse/NETOBSERV-268 to keep track.
/approve |
[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 |
This improves histogram user experience by adding buttons and keyboards shortcuts for the following actions:
To let the user better understand what's happening, the custom time range label has been replaced by actual values.
Also I suggest to decrease default limit to 50 as queries will be faster and this new method allows a better navigation. The best option for me would be to replace the current selector by a number input in a followup
based on #271