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

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

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

adamint
Copy link
Member

@adamint adamint commented Dec 10, 2024

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)

image

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

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?
Microsoft Reviewers: Open in CodeFlow

{
private async Task OnAllValuesCheckedChangedInternalAsync(bool? newAreAllVisible)
{
AreAllVisible = newAreAllVisible;
Copy link
Member Author

Choose a reason for hiding this comment

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

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
Copy link
Member

JamesNK commented Dec 11, 2024

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.

Comment on lines 13 to 14
AllValues="AllResourceTypes"
VisibleValues="VisibleResourceTypes"
Copy link
Member

Choose a reason for hiding this comment

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

I'm not a fan of having the available values and selected these stored in seperate collections. Having them stored seperately was ok when there was just one filter, but now that there are 3 - causing 6 collections - I think it needs to be improved.

Note: I didn't originally write the filtering here

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 collapsed these into one collection each. What do you think now?

@JamesNK
Copy link
Member

JamesNK commented Dec 11, 2024

Couple of examples of bugs.

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

db-type-not-displayed

Problems with unselecting resource types:

resource-type-not-displayed

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

@adamint
Copy link
Member Author

adamint commented Dec 12, 2024

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
Copy link
Member Author

adamint commented Dec 12, 2024

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.
image

…lthstatus

# Conflicts:
#	src/Aspire.Dashboard/wwwroot/css/app.css
@adamint adamint requested a review from JamesNK December 12, 2024 18:23
@adamint
Copy link
Member Author

adamint commented Dec 18, 2024

@JamesNK

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Filter resources with errors on dashboard
2 participants