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

Add resource details panel #1542

Merged
merged 10 commits into from
Jan 10, 2024
Merged

Add resource details panel #1542

merged 10 commits into from
Jan 10, 2024

Conversation

JamesNK
Copy link
Member

@JamesNK JamesNK commented Jan 4, 2024

Fixes #1208
Fixes #1489
Fixes #1584

READY FOR REVIEW

Playing around adding a resource panel. The resource grid is trying to display too much information and a panel allows less used content to be accessed there and removed from the grid.

  • Add details button to resources grid
  • Remove environment variables button from resources grid
  • Move logs button from resources grid to details panel
  • Add high-level resource values to panel. These are from returned resource properties
    • There is the concept of "known properties". These are displayed by default and have a friendly name in UI (can still hover over text to get the actual key).
    • Unknown properties are hidden by default. These are properties that aren't generally useful to the end user, e.g. the resource UID
    • The hide/show button toggles showing unknown properties.
    • This pattern will work well for custom resource types. Custom properties can be viewed in the UI.
  • Add resource endpoints/services to panel
  • Refactor resources panel to use an accordion like span panel
  • Remove various bits of detail from the resources grid
    • Container ID
    • Container ports
    • Container command/args
    • Process ID

Future work:

  • Display the resource panel by clicking the row instead of a details button. Will do in a follow up PR.

Demo (updated 2024/9/1):
resources-details-new

Note: The design is ready to review. Code is rough/unfinished in places. Will improve after design feedback.

Microsoft Reviewers: Open in CodeFlow

@JamesNK
Copy link
Member Author

JamesNK commented Jan 4, 2024

cc @leslierichardson95 @davidfowl

Copy link
Member

@drewnoakes drewnoakes left a comment

Choose a reason for hiding this comment

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

Please hold off on merging this. The classes you're using in the front end will no longer exist once #1476 is merged. I have new code that will produce the name/values you're producing here too, in a way that's extensible for arbitrary resource types.

@adamint
Copy link
Member

adamint commented Jan 4, 2024

I really like the redesigned panel. We're duplicating some information (ie the name, which appears in the panel title, and the other simple columns in the resource grid). We may not want to do that unless you foresee that the panel could be opened somewhere else in the dashboard in the future other than the grid?

@leslierichardson95
Copy link

I like the resources panel! +1 on Adam's comment about there being duplicate info though. While there may be some use cases for having duplicate fields, I worry that that may result in taking up more real estate on screen than needed.

@JamesNK
Copy link
Member Author

JamesNK commented Jan 8, 2024

Now that the resource refactor has happened, I think the way this UI works could be changed a bit. Each resource has a raw list of properties. To properly handle custom resources with unknown properties, this UI should display a list of properties.

I think by default the UI:

  • Displays known properties for known resource types (e.g. container ID for containers, source path for projects, etc) or all properties if the resource type is unknown
  • Clicking show all then shows all properties

@drewnoakes
Copy link
Member

If you want to give any of the properties custom treatment, we can model the required metadata for that in the ResourceType messages we send to the client.

@davidfowl
Copy link
Member

What's this change waiting on?

@JamesNK
Copy link
Member Author

JamesNK commented Jan 8, 2024

I made large refactors yesterday. Me finishing that work. Then moving hardcoded UI text to resource files.

@JamesNK
Copy link
Member Author

JamesNK commented Jan 9, 2024

This is now ready for final review.

@@ -25,13 +25,5 @@
}

@Resource.State
<UnreadLogErrorsBadge UnviewedCount="@GetUnviewedErrorCount(Resource)" Resource="@Resource"/>
<UnreadLogErrorsBadge UnviewedErrorCounts="UnviewedErrorCounts" Resource="@Resource" />
Copy link
Member Author

Choose a reason for hiding this comment

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

I made some changes to the unread errors component because I wanted to reuse it in the resource details component.

At least for now it isn't reused but I left the changes in because I think they're A Good Improvement.

@drewnoakes
Copy link
Member

Just pulled this down to take it for a spin. It's the end of my day so I'll look more tomorrow.

Initial impression is that it's a bit tricky to parse the grouping between sections. For example:

image

Adding some indentation here might help:

image

Another idea is to add a background to the expander headers.

@davidfowl
Copy link
Member

I like the background idea.

@kvenkatrajan
Copy link
Member

The new details panel is looking great. Approved.

I have gone ahead and added another issue for tracking the visibility on the expanded headers in lieu of not further increasing the size and scope of this PR : #1612

@adamint please do an accessibility pass after merge and add another issue if additional work needs to be done there. Thanks!

@JamesNK
Copy link
Member Author

JamesNK commented Jan 9, 2024

The accordion originally had a lot more stuff to separate sections. Different background colors for headers, borders, etc. I turned it off because it didn't look good.

I've slightly increased the gap between each section (4px to 8px) to help highlight them:
image

@JamesNK
Copy link
Member Author

JamesNK commented Jan 9, 2024

From offline: Will merge after @drewnoakes takes a look at the code.

Copy link
Member

@drewnoakes drewnoakes left a comment

Choose a reason for hiding this comment

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

Approving, but would like to see the ResourceProperty.DisplayName used rather than having hard-coded knowledge of different resource properties in the UI.

new KnownProperty(KnownProperties.Container.Command, Loc[Resources.Resources.ResourcesDetailsContainerCommandProperty]),
new KnownProperty(KnownProperties.Container.Args, Loc[Resources.Resources.ResourcesDetailsContainerArgumentsProperty]),
new KnownProperty(KnownProperties.Container.Ports, Loc[Resources.Resources.ResourcesDetailsContainerPortsProperty]),
];
Copy link
Member

Choose a reason for hiding this comment

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

We shouldn't need to hard-code known properties for each resource type here. The ResourceProperty message received via gRPC includes a display name field. The dashboard should avoid special knowledge about specific fields wherever possible.

It also means we don't have to keep this code up to date as properties are added/removed.

Copy link
Member Author

@JamesNK JamesNK Jan 10, 2024

Choose a reason for hiding this comment

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

We shouldn't need to hard-code known properties for each resource type here.

We don't need to, but it improves the experience. They're a balance between making all data for a resource available, while still having a good default experience.

Known properties server three purposes:

  1. By default, display a curated list of information that we think is important. For example, it's useful to know the state, while resource.uid is an internal value.
  2. Controlling order of information. Ensures the most useful information is first.
  3. Displaying a friendly text in the name column.

This is what it looks like without them:
image

Updating known properties is optional. Data is still available in the UI if not known. Updating UI to show new data is a normal part of app development.

Copy link
Member

@drewnoakes drewnoakes Jan 10, 2024

Choose a reason for hiding this comment

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

We should move the loc strings to the server side and remove this client-side reference data. All reference data that governs the UI should come from the back end so that we don't have to add explicit support for custom resources in the dashboard.

The UI can still preserve order, as the values are stored in an array rather than an associative collection.

Copy link
Member

Choose a reason for hiding this comment

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

@JamesNK JamesNK enabled auto-merge (squash) January 10, 2024 08:19
@JamesNK JamesNK merged commit 33331a0 into main Jan 10, 2024
8 checks passed
@JamesNK JamesNK deleted the jamesnk/resource-details branch January 10, 2024 08:45
@davidfowl
Copy link
Member

Running the dapr sample I noticed we didn't clean up executables. They still show the args and working directory.

image

@github-actions github-actions bot locked and limited conversation to collaborators Apr 26, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Display short container ID Show endpoints even if they aren't http Resource details panel on dashboard
6 participants