Allow filtering by resource state and health status, and update the filters based on visible grid items #6925

adamint · 2024-12-10T19:22:36Z

Description

Currently, you can filter resources by type, but it would also be useful to filter by health status or by resource state. I extracted the resource filter UI into its own component ResourceFilters and converted SelectResourceTypes into a component that was more generic and so could be reused for state and health status (SelectResourceOptions). This also meant other refactoring was necessary, such as creating reusable methods Get/SetFieldVisibility from the getter and setter logic previously in AreAllTypesVisible.

Fixes #1190

In a follow-up, we could persist these values in the URL (#3647)

(note that, for example, finished and no state have disappeared from available options because no resource with those values is in the grid anymore)

Checklist

Is this feature complete?
- Yes. Ready to ship.
- No. Follow-up changes expected.
Are you including unit tests for the changes and scenario tests if relevant?
- Yes
- No (largely just a refactor)
Did you add public API?
- Yes
  - If yes, did you have an API Review for it?
    - Yes
    - No
  - Did you add <remarks /> and <code /> elements on your triple slash comments?
    - Yes
    - No
- No
Does the change make any security assumptions or guarantees?
- Yes
  - If yes, have you done a threat model and had a security review?
    - Yes
    - No
- No
Does the change require an update in our Aspire docs?
- Yes
  - Is this introducing a breaking change?
    - Yes
      - Link to aspire-docs issue (please use this breaking-change template):
    - No
      - Link to aspire-docs issue (please use this doc-idea template):
- No

Microsoft Reviewers: Open in CodeFlow

…der options by name

… grid

src/Aspire.Dashboard/Components/Controls/SelectResourceOptions.razor

adamint · 2024-12-10T19:24:42Z

src/Aspire.Dashboard/Components/Controls/SelectResourceOptions.razor.cs

+{
+    private async Task OnAllValuesCheckedChangedInternalAsync(bool? newAreAllVisible)
+    {
+        AreAllVisible = newAreAllVisible;


Previously this line (and the logic in OnValueVisibilityChangedInternalAsync) was done in the callback but can be done directly in this component before invoking the callback.

JamesNK · 2024-12-11T01:28:00Z

What happens if there are a lot of states, resources types and health states? The popup should probably have a maximum height and a scoll bar to handle that situation nicely.

src/Aspire.Dashboard/Components/Controls/ResourceFilters.razor

src/Aspire.Dashboard/Components/Controls/SelectResourceOptions.razor

JamesNK · 2024-12-11T02:04:18Z

Couple of examples of bugs.

Database type isn't in list because its resource is nested. Expanding the parent makes it display:

Problems with unselecting resource types:

These are both caused by filtering available options based on visible resources.

adamint · 2024-12-12T17:26:17Z

Couple of examples of bugs.

Database type isn't in list because its resource is nested. Expanding the parent makes it display:

Problems with unselecting resource types:

These are both caused by filtering available options based on visible resources.

Reverted the change.

adamint · 2024-12-12T18:22:54Z

What happens if there are a lot of states, resources types and health states? The popup should probably have a maximum height and a scoll bar to handle that situation nicely.

Added a max height and scroll bar.

…lthstatus # Conflicts: # src/Aspire.Dashboard/wwwroot/css/app.css

adamint · 2024-12-18T20:59:31Z

@JamesNK

JamesNK · 2024-12-30T07:15:06Z

src/Aspire.Dashboard/Components/Controls/SelectResourceOptions.razor.cs

+        if (isVisible)
+        {
+            AllValues[value] = true;
+        }
+        else
+        {
+            AllValues[value] = false;
+        }


Suggested change

if (isVisible)

{

AllValues[value] = true;

}

else

{

AllValues[value] = false;

}

AllValues[value] = isVisible;

JamesNK · 2024-12-30T07:16:35Z

src/Aspire.Dashboard/Components/Controls/SelectResourceOptions.razor.cs

+        return areAllChecked
+            ? true
+            : areAllUnchecked
+                ? false
+                : null;


I don't like nested ternary statements. Make at least one of these an if.

JamesNK · 2024-12-30T07:18:08Z

src/Aspire.Dashboard/Components/Controls/SelectResourceOptions.razor.cs

+    private async Task OnAllValuesCheckedChangedInternalAsync(bool? newAreAllVisible)
+    {
+        SetCheckState(newAreAllVisible, AllValues);
+        await OnAllResourceTypesCheckedChangedAsync();
+    }


Suggested change

private async Task OnAllValuesCheckedChangedInternalAsync(bool? newAreAllVisible)

{

SetCheckState(newAreAllVisible, AllValues);

await OnAllResourceTypesCheckedChangedAsync();

}

private async Task OnAllValuesCheckedChangedInternalAsync(bool? newAreAllVisible)

{

SetCheckState(newAreAllVisible, Values);

await OnResourceTypesCheckedChangedAsync();

}

Don't need All in many of these names anymore (I think this suggestion is right). All should just be used when working with the all button now.

JamesNK · 2024-12-30T07:18:37Z

src/Aspire.Dashboard/Components/Controls/SelectResourceOptions.razor

+
+@code {
+    [Parameter, EditorRequired]
+    public required ConcurrentDictionary<TValue, bool> AllValues { get; set; }


Suggested change

public required ConcurrentDictionary<TValue, bool> AllValues { get; set; }

public required ConcurrentDictionary<TValue, bool> Values { get; set; }

JamesNK · 2024-12-30T07:23:07Z

src/Aspire.Dashboard/Components/Pages/Resources.razor.cs

-        }
+        return _resourceTypesToVisibility.TryGetValue(resource.ResourceType, out var typeVisible) && typeVisible
+               && _resourceStatesToVisibility.TryGetValue(GetStateOrDefaultText(resource.State, Loc), out var stateVisible) && stateVisible
+                && _resourceHealthStatusesToVisibility.TryGetValue(GetStateOrDefaultText(resource.HealthStatus?.Humanize(), Loc), out var healthStateVisible) && healthStateVisible


Suggested change

&& _resourceHealthStatusesToVisibility.TryGetValue(GetStateOrDefaultText(resource.HealthStatus?.Humanize(), Loc), out var healthStateVisible) && healthStateVisible

&& _resourceHealthStatusesToVisibility.TryGetValue(GetStateOrDefaultText(resource.HealthStatus?.Humanize(), Loc), out var healthStateVisible) && healthStateVisible

Have a local method for getting true/false from these dictionaries that avoids the need for TryGetValue.

JamesNK · 2024-12-30T07:31:18Z

src/Aspire.Dashboard/Components/Pages/Resources.razor

-                <FluentButton id="typeFilterButton" slot="end"
-                              Appearance="@(AreAllTypesVisible is true ? Appearance.Stealth : Appearance.Accent)"
+                <FluentButton id="resourceFilterButton" slot="end"
+                              Appearance="@(AreAllVisibleInAnyFilter ? Appearance.Stealth : Appearance.Accent)"


This doesn't seem to work anymore.

JamesNK · 2024-12-30T07:32:01Z

src/Aspire.Dashboard/Components/Pages/Resources.razor

+                              Title="@(AreAllVisibleInAnyFilter ? Loc[nameof(Dashboard.Resources.Resources.ResourcesTypeFilterAllVisible)] : Loc[nameof(Dashboard.Resources.Resources.ResourcesTypeFiltered)])"
+                              aria-label="@(AreAllVisibleInAnyFilter ? Loc[nameof(Dashboard.Resources.Resources.ResourcesTypeFilterAllVisible)] : Loc[nameof(Dashboard.Resources.Resources.ResourcesTypeFiltered)])"></FluentButton>


The resource text doesn't make sense anymore. It should be made generic to "has filters" vs "no filters"

JamesNK · 2024-12-30T07:42:02Z

src/Aspire.Dashboard/Components/Pages/Resources.razor.cs

+                _resourceStatesToVisibility.TryAdd(GetStateOrDefaultText(resource.State, Loc), preselectedVisibleResourceStates is null || preselectedVisibleResourceStates.Contains(resource.State ?? string.Empty));
+                 _resourceHealthStatusesToVisibility.TryAdd(GetStateOrDefaultText(resource.HealthStatus?.Humanize(), Loc), preselectedVisibleResourceHealthStates is null || preselectedVisibleResourceHealthStates.Contains(resource.HealthStatus?.Humanize() ?? string.Empty));


If there is no value then the key can be an empty string. It shouldn't be translated text.

Translated text should only be applied in the UI, and this isn't UI related code. Later, when you do show the value in the UI, the view can check if the string is empty and display the default text instead.

JamesNK · 2024-12-30T07:49:45Z

src/Aspire.Dashboard/wwwroot/css/app.css

@@ -734,3 +734,8 @@ fluent-data-grid-cell.no-ellipsis {
    vertical-align: text-bottom;
    margin-right: 3px
 }
+
+.resources-popup {


Suggested change

.resources-popup {

.resources-filter-popup {

JamesNK · 2024-12-30T07:53:16Z

src/Aspire.Dashboard/wwwroot/css/app.css

+
+.resources-popup {
+    max-height: 400px;
+    overflow-y: scroll;


This value always display a scrollbar (disabled when not needed). It should be auto

JamesNK · 2024-12-30T07:56:21Z

I think you should add some component tests to test these changes. We don't tests for resources page yet so you'll need to add some setup code for registering required services and JS calls, but there a lot of other pages already setup that you can follow.

Adam Ratzman added 6 commits December 10, 2024 11:16

Add stack, header for state

62de9e2

Add resource filter by state

bd9bbbc

Add resource filters component to share same code for mobile view, or…

42132a6

…der options by name

Refresh data immediately after search filter entry is changed

451fa5a

Filter out values in resource filter options that don't appear in the…

21abd03

… grid

update AreAllVisibleInAnyFilter to include health status

ec3d504

adamint requested review from JamesNK, drewnoakes and tlmii December 10, 2024 19:22

adamint commented Dec 10, 2024

View reviewed changes

src/Aspire.Dashboard/Components/Controls/SelectResourceOptions.razor Outdated Show resolved Hide resolved

adamint commented Dec 10, 2024

View reviewed changes

rename typeFilterButton -> resourceFilterButton

f5589cf

build-analysis bot mentioned this pull request Dec 10, 2024

restarted. Azure DevOps can't recover from restarts. dotnet/dnceng#3879

Open

3 tasks

JamesNK reviewed Dec 11, 2024

View reviewed changes

src/Aspire.Dashboard/Components/Controls/ResourceFilters.razor Outdated Show resolved Hide resolved

src/Aspire.Dashboard/Components/Controls/SelectResourceOptions.razor Outdated Show resolved Hide resolved

Adam Ratzman added 3 commits December 12, 2024 12:26

remove ResourceContainsValue

dcdf0dd

only use AllValues to hold check states

c04043e

add max height for popup and scrolling

c8aa3f2

Merge branch 'main' into dev/adamint/filter-resources-by-state-or-hea…

d2c6443

…lthstatus # Conflicts: # src/Aspire.Dashboard/wwwroot/css/app.css

adamint requested a review from JamesNK December 12, 2024 18:23

JamesNK requested changes Dec 30, 2024

View reviewed changes

dotnet-policy-service bot added the needs-author-action An issue or pull request that requires more info or actions from the author. label Dec 30, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow filtering by resource state and health status, and update the filters based on visible grid items #6925

Allow filtering by resource state and health status, and update the filters based on visible grid items #6925

adamint commented Dec 10, 2024 •

edited

Loading

adamint Dec 10, 2024

JamesNK commented Dec 11, 2024

JamesNK commented Dec 11, 2024

adamint commented Dec 12, 2024

adamint commented Dec 12, 2024

adamint commented Dec 18, 2024

JamesNK Dec 30, 2024

JamesNK Dec 30, 2024

JamesNK Dec 30, 2024

JamesNK Dec 30, 2024

JamesNK Dec 30, 2024

JamesNK Dec 30, 2024

JamesNK Dec 30, 2024

JamesNK Dec 30, 2024

JamesNK Dec 30, 2024 •

edited

Loading

JamesNK Dec 30, 2024

JamesNK Dec 30, 2024

JamesNK commented Dec 30, 2024

	public required ConcurrentDictionary<TValue, bool> AllValues { get; set; }
	public required ConcurrentDictionary<TValue, bool> Values { get; set; }

	&& _resourceHealthStatusesToVisibility.TryGetValue(GetStateOrDefaultText(resource.HealthStatus?.Humanize(), Loc), out var healthStateVisible) && healthStateVisible
	&& _resourceHealthStatusesToVisibility.TryGetValue(GetStateOrDefaultText(resource.HealthStatus?.Humanize(), Loc), out var healthStateVisible) && healthStateVisible

		Title="@(AreAllVisibleInAnyFilter ? Loc[nameof(Dashboard.Resources.Resources.ResourcesTypeFilterAllVisible)] : Loc[nameof(Dashboard.Resources.Resources.ResourcesTypeFiltered)])"
		aria-label="@(AreAllVisibleInAnyFilter ? Loc[nameof(Dashboard.Resources.Resources.ResourcesTypeFilterAllVisible)] : Loc[nameof(Dashboard.Resources.Resources.ResourcesTypeFiltered)])"></FluentButton>

		_resourceStatesToVisibility.TryAdd(GetStateOrDefaultText(resource.State, Loc), preselectedVisibleResourceStates is null \|\| preselectedVisibleResourceStates.Contains(resource.State ?? string.Empty));
		_resourceHealthStatusesToVisibility.TryAdd(GetStateOrDefaultText(resource.HealthStatus?.Humanize(), Loc), preselectedVisibleResourceHealthStates is null \|\| preselectedVisibleResourceHealthStates.Contains(resource.HealthStatus?.Humanize() ?? string.Empty));

Allow filtering by resource state and health status, and update the filters based on visible grid items #6925

Are you sure you want to change the base?

Allow filtering by resource state and health status, and update the filters based on visible grid items #6925

Conversation

adamint commented Dec 10, 2024 • edited Loading

Description

Checklist

Microsoft Reviewers: Open in CodeFlow

Choose a reason for hiding this comment

JamesNK commented Dec 11, 2024

JamesNK commented Dec 11, 2024

adamint commented Dec 12, 2024

adamint commented Dec 12, 2024

adamint commented Dec 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JamesNK Dec 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JamesNK commented Dec 30, 2024

adamint commented Dec 10, 2024 •

edited

Loading

JamesNK Dec 30, 2024 •

edited

Loading