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-1098: Show scope as slider #344

Merged
merged 4 commits into from
Jul 25, 2023
Merged

NETOBSERV-1098: Show scope as slider #344

merged 4 commits into from
Jul 25, 2023

Conversation

jotak
Copy link
Member

@jotak jotak commented Jun 14, 2023

Borrow slider from patternfly, and adapt for vertical display

image

@jotak
Copy link
Member Author

jotak commented Jun 14, 2023

Opening as draft for now - we may clean the Slider component more to keep only what we need

@codecov
Copy link

codecov bot commented Jun 14, 2023

Codecov Report

Merging #344 (3c1526d) into main (b9287d0) will decrease coverage by 1.63%.
The diff coverage is 11.24%.

@@            Coverage Diff             @@
##             main     #344      +/-   ##
==========================================
- Coverage   58.51%   56.88%   -1.63%     
==========================================
  Files         158      162       +4     
  Lines        7097     7337     +240     
  Branches      868      902      +34     
==========================================
+ Hits         4153     4174      +21     
- Misses       2698     2917     +219     
  Partials      246      246              
Flag Coverage Δ
uitests 58.30% <11.24%> (-1.46%) ⬇️
unittests 52.71% <ø> (-2.10%) ⬇️

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

Impacted Files Coverage Δ
web/src/components/slider/Slider.tsx 5.33% <5.33%> (ø)
web/src/components/scope-slider/scope-slider.tsx 45.45% <45.45%> (ø)
web/src/components/slider/SliderStep.tsx 71.42% <71.42%> (ø)
...c/components/netflow-topology/netflow-topology.tsx 68.75% <100.00%> (+1.00%) ⬆️

... and 3 files with indirect coverage changes

@jotak jotak changed the title Show scope as slider NETOBSERV-1098: Show scope as slider Jun 15, 2023
@openshift-ci-robot
Copy link
Collaborator

openshift-ci-robot commented Jun 15, 2023

@jotak: This pull request references NETOBSERV-1098 which is a valid jira issue.

In response to this:

Borrow slider from patternfly, and adapt for vertical display

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@openshift-ci-robot
Copy link
Collaborator

openshift-ci-robot commented Jun 15, 2023

@jotak: This pull request references NETOBSERV-1098 which is a valid jira issue.

In response to this:

Borrow slider from patternfly, and adapt for vertical display

image

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@jotak jotak force-pushed the slider branch 3 times, most recently from d2c3e61 to 7634e18 Compare June 23, 2023 07:28
@jotak jotak marked this pull request as ready for review June 23, 2023 08:52
@jotak jotak requested a review from jpinsonneau June 23, 2023 08:52
@jotak
Copy link
Member Author

jotak commented Jun 23, 2023

@jpinsonneau @andrew-ronaldson @stleerh

This is ready for review.
I know the approach is not perfect, in several ways:

  • slider relies on a hacky rotation: ideally, something clean should be implemented upstream in PF
  • @stleerh gave some feedback that texts can be confusing / he had a suggestion to use images instead. Or maybe we could find icons.
  • We could probably spend time discussing if there are better placements, if another component would better fit, etc.

But given how useful this is, tbh, if we don't find any blocking issue / regression, I'd be in favour of merging & iterating via a follow-up rather than holding for potentially a long time.

About the Slider component included in this PR, it's copied from PF and slightly amended, in 2 different commits so that we can retrieve the diff & rebase more easily from upstream when needed.
Another approach could be to change it more aggressively and remove everything we don't need - pro would be a simpler component, cons would be more diverging from upstream hence less easy to re-apply upstream patches if any.

@jotak jotak requested a review from OlivierCazade July 18, 2023 14:29
OlivierCazade
OlivierCazade previously approved these changes Jul 19, 2023
Copy link
Collaborator

@OlivierCazade OlivierCazade left a comment

Choose a reason for hiding this comment

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

LGTM

@jotak
Copy link
Member Author

jotak commented Jul 21, 2023

For information, I opened an enhancement request on patternfly for an upstream (and clean) vertical slider implementation: https://github.com/orgs/patternfly/discussions/5775

@memodi
Copy link
Contributor

memodi commented Jul 21, 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 Jul 21, 2023
@memodi
Copy link
Contributor

memodi commented Jul 21, 2023

@jotak - image building is failing here with below error:

82.84 ./components/scope-slider/scope-slider.tsx 3:9-20
82.84 [tsl] ERROR in /opt/app-root/web/src/components/scope-slider/scope-slider.tsx(3,10)
82.84       TS2305: Module '"../../model/flow-query"' has no exported member 'MetricScope'.

Borrow slider from patternfly, and adapt for vertical display
@openshift-ci
Copy link

openshift-ci bot commented Jul 24, 2023

New changes are detected. LGTM label has been removed.

@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 Jul 24, 2023
@jotak
Copy link
Member Author

jotak commented Jul 24, 2023

@memodi yes, I didn't see, post-rebase issues, they should be fixed

@jotak jotak added the ok-to-test To set manually when a PR is safe to test. Triggers image build on PR. label Jul 24, 2023
@github-actions
Copy link

New image:
quay.io/netobserv/network-observability-console-plugin:a16f99f

It will expire after two weeks.

To deploy this build, run from the operator repo, assuming the operator is running:

USER=netobserv VERSION=a16f99f make set-plugin-image

@memodi
Copy link
Contributor

memodi commented Jul 24, 2023

/label qe-approved
tested with the slider and with swap/bnf buttons. Also tested with existing advance options.

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

jotak commented Jul 25, 2023

hey @jpinsonneau are you ok with this plan: merge this and track the patternfly ticket for an upstream implementation?

@jpinsonneau
Copy link
Contributor

hey @jpinsonneau are you ok with this plan: merge this and track the patternfly ticket for an upstream implementation?

sure, as it's still possible to manage it in the advance options I don't think it's a blocker to merge as is 😉
Thanks @jotak

@jotak
Copy link
Member Author

jotak commented Jul 25, 2023

/approve

@openshift-ci
Copy link

openshift-ci bot commented Jul 25, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jotak

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

@jotak jotak merged commit dccb069 into netobserv:main Jul 25, 2023
9 of 10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved jira/valid-reference 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.

None yet

5 participants