-
Notifications
You must be signed in to change notification settings - Fork 22
NETOBSERV-1764 frontend code styling followup #551
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
Conversation
Skipping CI for Draft Pull Request. |
20762fd
to
1649fd8
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #551 +/- ##
==========================================
+ Coverage 56.10% 56.34% +0.24%
==========================================
Files 183 189 +6
Lines 9098 9155 +57
Branches 1185 1186 +1
==========================================
+ Hits 5104 5158 +54
- Misses 3623 3626 +3
Partials 371 371
Flags with carried forward coverage won't be shown. Click here to find out more.
|
300128d
to
1ad57be
Compare
/ok-to-test |
New image: It will expire after two weeks. To deploy this build, run from the operator repo, assuming the operator is running: USER=netobserv VERSION=d531b66 make set-plugin-image |
Fixed column issue reported in jira in 7e3cc4d |
New image: It will expire after two weeks. To deploy this build, run from the operator repo, assuming the operator is running: USER=netobserv VERSION=092a2ad make set-plugin-image |
Reran the tests and all tests are now passing |
Rebased without changes @jotak I hope I didn't break your changes here 👼 If you can have a quick check I would appreciate |
New image: It will expire after two weeks. To deploy this build, run from the operator repo, assuming the operator is running: USER=netobserv VERSION=e2c87b0 make set-plugin-image |
@@ -0,0 +1,406 @@ | |||
import { K8sModel } from '@openshift-console/dynamic-plugin-sdk'; |
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.
nit: can we name the file "netflow-traffic-drawer.tsx" so it matches the component name?
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.
done in 7ecf824
Tested and I didn't find any regression |
Ideally, as part of this refactoring, it would be nice to move all the fetching functions in their respective sub-component (NetflowOverview, NetflowTable ...) ; so NetflowTraffic would only be responsible for doing the "switch(selectedView)", leaving the other components specialized in their domain, responsible both for fetching and displaying data. Currently, even with this refactoring, NetflowTraffic is still centralizing and dispatching everything, this is what I was hopeful we would be able to break |
1a3e846
to
7ecf824
Compare
Yes that's something we can do using the |
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED Approval requirements bypassed by manually added approval. This pull-request has been approved by: 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 |
Description
Split netflow-traffic code that became too big for a single file
tabs
folder with an additionnal tabs container componentDependencies
#541
Checklist
If you are not familiar with our processes or don't know what to answer in the list below, let us know in a comment: the maintainers will take care of that.