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-634 UI Consistency: Side panel styling #214

Merged
merged 1 commit into from
Nov 15, 2022

Conversation

jpinsonneau
Copy link
Contributor

@jpinsonneau jpinsonneau commented Oct 20, 2022

Side panel updates:

  • wording / styling
  • tabs (Details / Raw JSON / Metrics)
  • sticky header (scroll is under the selected tab)
  • expandable groups for Source / Destination
  • added Namespace & Node infos when available on topology

image
image
image

image
image
image

@jpinsonneau
Copy link
Contributor Author

One interesting thing would be to add long descriptions on each field like:
image

However there is no value if we don't add particular details. I guess we could start from https://www.iana.org/assignments/ipfix/ipfix.xhtml and update according to our usage.

Can someone help me to write these ? Then I would be happy to implement !
@stleerh @jotak

@jpinsonneau jpinsonneau changed the title UI Consistency: Side panel styling NETOBSERV-634 UI Consistency: Side panel styling Oct 20, 2022
@andrew-ronaldson
Copy link

This looks good! The only thing I would change if it's possible is the Side panel Title. Can it be the resource name (P) dns-pod-blah-blah-blah-24 instead of "Pod"?

@jpinsonneau
Copy link
Contributor Author

This looks good! The only thing I would change if it's possible is the Side panel Title. Can it be the resource name (P) dns-pod-blah-blah-blah-24 instead of "Pod"?

Yes I can do that when you click on a single item @andrew-ronaldson but what about edges (source + destination) ?
It's also the same for table row 🤔

@jotak
Copy link
Member

jotak commented Oct 24, 2022

One interesting thing would be to add long descriptions on each field like

It could be nice. Some fields are pretty obvious (pod, kind, namespace...) other are less (direction, collection latency..) , we just set a description on those who aren't obvious? (or an info icon + tooltip?) It's also to avoid making the user having to scroll to much.

@jotak
Copy link
Member

jotak commented Oct 24, 2022

Also, I wonder if we need to keep the "kind / owner kind" fields as they are already displayed as icons

@jotak
Copy link
Member

jotak commented Oct 24, 2022

/lgtm
tested and reviewed
@jpinsonneau do you want to try your suggestions in this PR or later?

@jotak
Copy link
Member

jotak commented Oct 24, 2022

This looks good! The only thing I would change if it's possible is the Side panel Title. Can it be the resource name (P) dns-pod-blah-blah-blah-24 instead of "Pod"?

Yes I can do that when you click on a single item @andrew-ronaldson but what about edges (source + destination) ? It's also the same for table row thinking

+1 with Andrew's suggestion.
Do you think the full edge name "pod-xxx to pod-yyy" would be too long for a title?

@jpinsonneau
Copy link
Contributor Author

/lgtm tested and reviewed @jpinsonneau do you want to try your suggestions in this PR or later?

I have created a followup so we can really focus on the content
https://issues.redhat.com/browse/NETOBSERV-671

@jpinsonneau
Copy link
Contributor Author

+1 with Andrew's suggestion. Do you think the full edge name "pod-xxx to pod-yyy" would be too long for a title?

Yes this become too long and we will need to truncate
image

Also remember the side panel is resizable so in some cases we can have a really small space here

@jpinsonneau
Copy link
Contributor Author

rebased + added ResourceLink on title for node selection
image

@Amoghrd
Copy link
Contributor

Amoghrd commented Oct 24, 2022

/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 Oct 24, 2022
@github-actions
Copy link

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

@Amoghrd
Copy link
Contributor

Amoghrd commented Oct 25, 2022

/qe-approved

@andrew-ronaldson
Copy link

Can we match the heading size on the sidebar title?

Screen Shot 2022-10-24 at 10 02 22 AM

@stleerh
Copy link
Contributor

stleerh commented Oct 26, 2022

I want to throw this out there to see what others think.  The Details section is very long.  What if the field name and value were combined on one row and only wraps if it doesn't fit.  Would that make it easier to read?  IMO, the use case for this section is to view fields that are not in the table or to copy the data (e.g. to report an issue). This would be different than how the Pod section shows it, which is on separate rows.

@stleerh
Copy link
Contributor

stleerh commented Oct 26, 2022

One interesting thing would be to add long descriptions on each field like

It could be nice. Some fields are pretty obvious (pod, kind, namespace...) other are less (direction, collection latency..) , we just set a description on those who aren't obvious? (or an info icon + tooltip?) It's also to avoid making the user having to scroll to much.

I agree.  The ones that I don't think are obvious are: Start Time, End Time, Owner, Owner Kind, Direction, Duration, Collection Time, and Collection Latency.

@jpinsonneau
Copy link
Contributor Author

I want to throw this out there to see what others think.  The Details section is very long.  What if the field name and value were combined on one row and only wraps if it doesn't fit.

I like the idea and that would work fine with large panels like this:
image

However, as soon as the panel is normal / small I would move back all values under their titles for consistency.
Does it makes sense ? @stleerh @andrew-ronaldson

I can create a followup for this about "responsive UI" with similar behaviors in other views

@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 Oct 27, 2022
@jpinsonneau
Copy link
Contributor Author

Can we match the heading size on the sidebar title?

Fixed title size:
image

Also took the opportunity to add a message on External / Unknown nodes:
image

Check commit 41e1f95

@jotak
Copy link
Member

jotak commented Oct 27, 2022

@stleerh I fully agree, if we can avoid too much scrolling and having more info in one sight, I like it :)

@andrew-ronaldson
Copy link

It would be good to have a PF empty state for those unknown nodes.

The title font-weight on the linked version is smaller than the non-linked ones. Sizing looks good though.

Based on the details side panel of the dev console it looks like we can reduce the spacing between field labels and info. I see 14px font-size and 1.66em line-height. Also they have some side-by-side content for shorter fields.

Screen Shot 2022-10-27 at 10 40 32 AM

@jpinsonneau
Copy link
Contributor Author

Rebased branch, no changes on my side

@Amoghrd
Copy link
Contributor

Amoghrd commented Nov 10, 2022

/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 Nov 10, 2022
@github-actions
Copy link

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

@Amoghrd
Copy link
Contributor

Amoghrd commented Nov 10, 2022

/qe-approved

@jotak
Copy link
Member

jotak commented Nov 15, 2022

/lgtm
Just a remark: we need to care about the separation of concerns and all this kind of things / code quality; the element-panel component was already doing multiple various things, now it's growing a little bit, whereas I think it's a very good candidate for more composition.
I'll do it via https://issues.redhat.com/browse/NETOBSERV-474 (I've already started it) ; but that's something to care about continuously.

@openshift-ci openshift-ci bot added the lgtm label Nov 15, 2022
@jpinsonneau
Copy link
Contributor Author

/approve

@openshift-ci
Copy link

openshift-ci bot commented Nov 15, 2022

[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

@openshift-merge-robot openshift-merge-robot merged commit dce1518 into netobserv:main Nov 15, 2022
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants