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-268 frontend code styling #541

Merged
merged 10 commits into from
Jun 27, 2024

Conversation

jpinsonneau
Copy link
Contributor

@jpinsonneau jpinsonneau commented Jun 13, 2024

Description

  • Sort imports
  • Constants naming
  • Usage of vs css display: flex
  • Usage of
  • Define interface for props every time
  • Move non component files to utils (filters-helper.ts, metrics-helper.tsx)
  • Split netflow-traffic code that became too big for a single file => will be done in a followup
  • Split other files (panels / contents)

Dependencies

n/a

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.

  • Is this PR backed with a JIRA ticket? If so, make sure it is written as a title prefix (in general, PRs affecting the NetObserv/Network Observability product should be backed with a JIRA ticket - especially if they bring user facing changes).
  • Does this PR require product documentation?
    • If so, make sure the JIRA epic is labelled with "documentation" and provides a description relevant for doc writers, such as use cases or scenarios. Any required step to activate or configure the feature should be documented there, such as new CRD knobs.
  • Does this PR require a product release notes entry?
    • If so, fill in "Release Note Text" in the JIRA.
  • Is there anything else the QE team should know before testing? E.g: configuration changes, environment setup, etc.
    • If so, make sure it is described in the JIRA ticket.
  • QE requirements (check 1 from the list):
    • Standard QE validation, with pre-merge tests unless stated otherwise.
    • Regression tests only (e.g. refactoring with no user-facing change).
    • No QE (e.g. trivial change with high reviewer's confidence, or per agreement with the QE team).

Copy link

openshift-ci bot commented Jun 13, 2024

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

Copy link

codecov bot commented Jun 13, 2024

Codecov Report

Attention: Patch coverage is 70.83812% with 254 lines in your changes missing coverage. Please review.

Project coverage is 56.43%. Comparing base (e3bdd3d) to head (1caa117).
Report is 3 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #541      +/-   ##
==========================================
- Coverage   56.56%   56.43%   -0.14%     
==========================================
  Files         173      183      +10     
  Lines        9042     9079      +37     
  Branches     1183     1182       -1     
==========================================
+ Hits         5115     5124       +9     
- Misses       3561     3589      +28     
  Partials      366      366              
Flag Coverage Δ
uitests 58.01% <70.83%> (+0.05%) ⬆️
unittests 52.03% <ø> (-0.62%) ⬇️

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

Files Coverage Δ
web/src/api/loki.ts 85.71% <100.00%> (ø)
web/src/components/__tests-data__/filters.ts 100.00% <100.00%> (ø)
web/src/components/__tests-data__/flows.ts 100.00% <100.00%> (ø)
web/src/components/__tests-data__/metrics.ts 100.00% <100.00%> (ø)
web/src/components/alerts/banner.tsx 43.75% <100.00%> (-3.31%) ⬇️
web/src/components/alerts/fetcher.tsx 55.00% <100.00%> (ø)
web/src/components/dropdowns/group-dropdown.tsx 70.58% <100.00%> (-0.85%) ⬇️
web/src/components/dropdowns/layout-dropdown.tsx 89.65% <100.00%> (-0.35%) ⬇️
.../src/components/dropdowns/metric-type-dropdown.tsx 61.76% <100.00%> (-1.10%) ⬇️
...components/dropdowns/overview-display-dropdown.tsx 63.63% <100.00%> (-3.04%) ⬇️
... and 93 more

... and 2 files with indirect coverage changes

Copy link
Member

@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.

nice!
/lgtm

(edit: ah that's still a draft ... but feel free to merge and open another one if this can avoid you troubles rebasing if kept open for too long .. as you want)

@openshift-ci openshift-ci bot removed the lgtm label Jun 17, 2024
@jpinsonneau jpinsonneau marked this pull request as ready for review June 17, 2024 13:52
@jpinsonneau
Copy link
Contributor Author

nice! /lgtm

(edit: ah that's still a draft ... but feel free to merge and open another one if this can avoid you troubles rebasing if kept open for too long .. as you want)

Sorry @jotak I had other changes in my list
I think we should merge that first and I'll work on the netflow-traffic page split right after

Copy link
Contributor

@stleerh stleerh left a comment

Choose a reason for hiding this comment

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

Just one minor comment.  Everything else /lgtm.

margin-top: 0 !important;
margin-right: 0 !important;
margin-bottom: 0 !important;
margin-left: 0 !important;
Copy link
Contributor

Choose a reason for hiding this comment

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

margin:0 !important; is the shortcut for lines 244-247 so you can remove lines 244-247.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure, addressed in 1120718

@jotak
Copy link
Member

jotak commented Jun 26, 2024

@jpinsonneau LGTM, just leaving you checking the failing cypress test and Steven's comment

@jpinsonneau
Copy link
Contributor Author

@jpinsonneau LGTM, just leaving you checking the failing cypress test and Steven's comment

not sure what was causing the issue. Let's see if it happens again

@jpinsonneau
Copy link
Contributor Author

@memodi any idea on what could cause the cypress error ?

  netflow-topology
    ✓ displays topology and namespaces (14093ms)
    1) find network observability namespace
Warning: vkCreateInstance: Found no drivers!
Warning: vkCreateInstance failed with VK_ERROR_INCOMPATIBLE_DRIVER
    at CheckVkSuccessImpl (../../third_party/dawn/src/dawn/native/vulkan/VulkanError.cpp:88)
    at CreateVkInstance (../../third_party/dawn/src/dawn/native/vulkan/BackendVk.cpp:458)
    at Initialize (../../third_party/dawn/src/dawn/native/vulkan/BackendVk.cpp:344)
    at Create (../../third_party/dawn/src/dawn/native/vulkan/BackendVk.cpp:266)
    at operator() (../../third_party/dawn/src/dawn/native/vulkan/BackendVk.cpp:521)
    ✓ update options (62983ms)
  2 passing (2m)
  1 failing
  1) netflow-topology
       find network observability namespace:
     AssertionError: Timed out retrying after 10000ms: Expected to find element: `.pf-c-chart`, but never found it.
      at Context.eval (webpack://netobserv-plugin/./cypress/e2e/topology/topology.spec.ts:41:26)

@jpinsonneau
Copy link
Contributor Author

Seems like 1caa117 fixed it 😸

@memodi
Copy link
Contributor

memodi commented Jun 26, 2024

Seems like 1caa117 fixed it 😸

glad, you were able to figure this out. I'd not have idea. Perhaps we should enable video recording and archiving images from failed runs to the build logs?

@jotak
Copy link
Member

jotak commented Jun 26, 2024

/lgtm

Copy link

openshift-ci bot commented Jun 27, 2024

[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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-merge-bot openshift-merge-bot bot merged commit 56ada60 into netobserv:main Jun 27, 2024
11 checks passed
@memodi
Copy link
Contributor

memodi commented Jun 27, 2024

cc @Amoghrd this has been merged, were there any regression test impact?

@Amoghrd
Copy link
Contributor

Amoghrd commented Jun 27, 2024

Havent tested it. Will run regression. Was waiting for a reply on JIRA

@Amoghrd
Copy link
Contributor

Amoghrd commented Jun 27, 2024

Regression is passing cypress-tests#664 with some flakes. All passed on manual rerun. But table_queryopts and netflow_table tests required changes; Made here: openshift-tests-private#17547

jotak pushed a commit to jotak/network-observability-console-plugin that referenced this pull request Jul 15, 2024
* prettier sort imports

* sort imports + constants naming

* flex + flexitem

* TextContent and Text usage

* interfaces for props

* move helpers + voronoi as component

* split dropdowns / panels when possible

* fixes

* removed unecessary CSS

* fix voronoi container
jotak pushed a commit that referenced this pull request Jul 16, 2024
* prettier sort imports

* sort imports + constants naming

* flex + flexitem

* TextContent and Text usage

* interfaces for props

* move helpers + voronoi as component

* split dropdowns / panels when possible

* fixes

* removed unecessary CSS

* fix voronoi container
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants