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

Combined Resources View #987

Merged
merged 3 commits into from
Nov 22, 2023
Merged

Conversation

tlmii
Copy link
Member

@tlmii tlmii commented Nov 21, 2023

Resolves #892

This differs a bit from the screenshots that I showed in #892 (comment) for a few reasons that I'll try to address below as I detail the changes.

The GH diff viewer treated Index.razor -> Resources.razor as a rename rather than a delete and add, and that is somewhat helpful in seeing what actually changed from the old pages to the new single page.

  1. Added a single page at / that shows all resources, regardless of type. Default ordering is by type ascending and then by name ascending.
  2. Added a Type column in the first position. This is different from the screenshots where I added an icon-only column. Two reasons for this: First, visual scanning of the icons was hard. They're just not big enough and different enough. Second, there are some rendering issues with an icon-only column that would need to be sorted out. I'll file an issue separately for that.
  3. Added a filter button to the toolbar that allows you to specify which of Projects/Containers/Executables (or all) are shown at a given time. Right now this is not remembered across page loads, but we could add that later if so desired (this PR is already big enough).
  4. Combined Name + PID (projects/executables) and Name + Container ID (containers) in the name column. Container ID is shortened to first 8 characters, but has the copy button from the properties grid that will copy the full value. PID/CID are slightly greyed out to differentiate from the name
  5. Combined Source Location (Projects), Executable Path + Arguments + Working Directory (Executables) and Container Image + Ports (Containers) into "Source" column (naming per suggestion in the issue). Exe path + arguments is combined to look like a full command line, working directory is on a second line. Arguments and Ports are slightly greyed out to separate them from the item before it on the same line. The two lines for executables both have tooltips so you can see the entire thing if the display gets truncated with ellipses. I think we can likely tweak this display going forward (especially executables) as people use it more, but this seemed like an inoffensive starting point.
  6. Deleted the individual resource pages from the project and from the left menu, replacing them in the menu with one Resources entry with a new icon (@leslierichardson95 we are definitely going to need an overall icon review)
  7. Changed DashboardViewModelService to have an "all resources" change tracker in addition to the individual type change trackers. The individual ones are still used on the logs pages. If we end up combining the logs pages (see Consider merging Project Logs and Container Logs pages #176) then we could potentially revisit that and streamline it to just the single change tracker.
  8. The Environment Variables/Details View didn't change as part of this. All the existing data from the grids is still available (albeit combined w/ other data in some cases) in the new grid, so the details view still just shows the environment variables and their values

image
image
image
image
image

@DamianEdwards
Copy link
Member

This looks fantastic @tlmii! Did you try having a separate "ID" column rather than appending it to the name? Just curious how that looks and feels for quickly scanning.

@dbreshears
Copy link
Contributor

Looks great! Might be overlooking, but when results are filtered by Resource Type, is there an easy way to tell they are filtered?

@mmitche
Copy link
Member

mmitche commented Nov 21, 2023

/azp where

Copy link

Azure DevOps orgs getting events for this repository:

@tlmii
Copy link
Member Author

tlmii commented Nov 21, 2023

This looks fantastic @tlmii! Did you try having a separate "ID" column rather than appending it to the name? Just curious how that looks and feels for quickly scanning.

@DamianEdwards I did, and it was fine. But the minimum width required for a column and the small amount of data in it for the PID or shortened container ID seemed a bit wasteful when other columns were ending up truncated very quickly. The scanning was definitely a bit better, though the PID/CID combined into one column still leaves some visual scanning issues. I can bring that back if you'd prefer, though.

@DamianEdwards
Copy link
Member

@tlmii I wouldn't block on it now. I imagine we'll be making lots of changes to this view for a while yet 😄

Copy link
Member

@DamianEdwards DamianEdwards left a comment

Choose a reason for hiding this comment

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

Approving from a purely "product" point of view. I'll let others handle the code 😄
Can't wait to get this change in!

@JamesNK
Copy link
Member

JamesNK commented Nov 22, 2023

image

Why is "All" checked if "Project" is unchecked? It would be nice if the UI showed that half-checked box (a dash or square?) in "All". See https://developer.mozilla.org/en-US/docs/Web/CSS/:indeterminate

I don't think FluentUI supports it though. cc @vnbaaij

image

The source column is really large. It looks fine in this screenshot, but a lot of people will have small screens. They'll see a lot of truncated columns. I wonder if there is something we can do to truncate source paths in a more intelligent way so we can reduce the source column size.

Today the content is always trimmed at the end. We really want the opposite. For example, trimming c:\dev\aspire\whatever\project\Dashboard.csproj:

  • Before: c:\dev\aspire\whatever\project\Dash...
  • After: c:\..\whatever\project\Dashboard.csproj

This feels like a common problem. Is there a JS library for truncating paths?

Could be a future issue.

@davidfowl
Copy link
Member

Maybe just get rid of "All".

@mitchdenny
Copy link
Member

Definitely don't block on this but I kind of like the idea of a card view.

Copy link
Member

@davidfowl davidfowl left a comment

Choose a reason for hiding this comment

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

Merge it!

@tlmii
Copy link
Member Author

tlmii commented Nov 22, 2023

Why is "All" checked if "Project" is unchecked? It would be nice if the UI showed that half-checked box (a dash or square?) in "All". See https://developer.mozilla.org/en-US/docs/Web/CSS/:indeterminate

I don't think FluentUI supports it though. cc @vnbaaij

Its not exposed in the blazor component, and its only awkwardly exposed in the underlying web component. But Fluent UI does actually support it. I figured out how to get it working:

image

The source column is really large. It looks fine in this screenshot, but a lot of people will have small screens. They'll see a lot of truncated columns. I wonder if there is something we can do to truncate source paths in a more intelligent way so we can reduce the source column size.

Today the content is always trimmed at the end. We really want the opposite. For example, trimming c:\dev\aspire\whatever\project\Dashboard.csproj:

  • Before: c:\dev\aspire\whatever\project\Dash...
  • After: c:\..\whatever\project\Dashboard.csproj

This feels like a common problem. Is there a JS library for truncating paths?

Could be a future issue.

Agree completely on this. I debated for a bit seeing if we could do something where we only showed what was different between the projects. E.g:

  • Before: c:\dev\aspire\whatever\project1\project1.csproj, c:\dev\aspire\whatever\project2\project2.csproj
  • After: project1\project1.csproj, project2\project2.csproj

But I figured it warranted some more investigation into the standard/common way of doing this (as well as what libraries are available) and that didn't fit well with this PR. I'll file another issue.

@tlmii
Copy link
Member Author

tlmii commented Nov 22, 2023

Looks great! Might be overlooking, but when results are filtered by Resource Type, is there an easy way to tell they are filtered?

I changed it so the button is highlighted if anything is filtered out, e.g.:

image

And I added a title and an aria-label to the button that says whether it is filtered or not. This may not be enough long-term but I think its a good start.

@tlmii tlmii enabled auto-merge (squash) November 22, 2023 19:20
@tlmii tlmii merged commit ea05b9a into dotnet:main Nov 22, 2023
3 checks passed
@tlmii tlmii deleted the dev/combined-resources-view branch November 22, 2023 20:21
<FluentNavLink Icon="@(new Icons.Regular.Size24.BinFull())" Href="/Containers">Containers</FluentNavLink>
<FluentNavLink Icon="@(new Icons.Regular.Size24.AppGeneric())" Href="/Executables">Executables</FluentNavLink>
<FluentNavLink Icon="@(new Icons.Regular.Size24.AppFolder())" Href="/" Match="NavLinkMatch.All">Resources</FluentNavLink>
@* <FluentNavLink Icon="@(new Icons.Regular.Size24.BinFull())" Href="/Containers">Containers</FluentNavLink>
Copy link
Member

Choose a reason for hiding this comment

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

Remove

private async Task WriteProjectChange(ProjectViewModel projectViewModel, ObjectChangeType changeType = ObjectChangeType.Modified)
{
await _projectViewModelChangesChannel.Writer.WriteAsync(
new ResourceChanged<ProjectViewModel>(changeType, projectViewModel), _cancellationToken)
Copy link
Member

Choose a reason for hiding this comment

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

can create resourechanged only once and reuse.

Copy link
Member Author

Choose a reason for hiding this comment

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

The generic type is different (ResourceChanged<ProjectViewModel> vs ResourceChanged<ResourceViewModel>) so we can't do that. I guess maybe we could add an IResourceChanged and use generic variance to make it work? But that seemed outside the scope.

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.

Show all resources on a single page in the dashboard
9 participants