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

Generate resource type filters from data #1221

Merged
merged 4 commits into from
Dec 7, 2023

Conversation

drewnoakes
Copy link
Member

Note that currently that data is hard coded, but the new upcoming API work will allow the server to tell the client what types to expect.

Note that currently that data is hard coded, but the new upcoming API work will allow the server to tell the client what types to expect.
Comment on lines +6 to +9
internal static class StringComparers
{
public static StringComparer ResourceType => StringComparer.Ordinal;
}
Copy link
Member Author

Choose a reason for hiding this comment

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

This is a convention I've seen used in a few places, giving semantic names to different domains of strings, ensuring they're compared in a uniform way.

<FluentCheckbox @bind-Value="_areProjectsVisible" Label="Projects" @bind-Value:after="HandleTypeFilterTypeChanged" />
<FluentCheckbox @bind-Value="_areContainersVisible" Label="Containers" @bind-Value:after="HandleTypeFilterTypeChanged" />
<FluentCheckbox @bind-Value="_areExecutablesVisible" Label="Executables" @bind-Value:after="HandleTypeFilterTypeChanged" />
<FluentCheckbox
Copy link
Member

Choose a reason for hiding this comment

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

Looks like you got rid of the indeterminate (i.e. "some are checked some are not") state? Was that an intentional change?

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 had the impression it wasn't actually showing intederminate state in the previous code because of ShowIndeterminate="false", but maybe that's just clearing indeterminate state if present.

I will reinstate the indeterminate state.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, the naming is a little confusing. ShowIndeterminate actually controls whether the user can toggle between on/off and indeterminate with mouse clicks. We don't want that, so it's false. ThreeState parameter controls whether the component will even display the three states in any situation (including programmatic, which we use), so that one was true.

You can see some docs and examples here if it helps: https://www.fluentui-blazor.net/Checkbox#fluentcheckbox-class_properties

Copy link
Member

Choose a reason for hiding this comment

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

To clarify the existing behavior:

The "All" checkbox would automatically toggle to checked (if all other checkboxes were checked), unchecked (if no other checkboxes were checked) or indeterminate (any other combination of checks).
The user clicking it wouldn't ever toggle it to indeterminate, just between checked/unchecked - and those actions would impact all the other checkboxes as appropriate.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, I think it'll be straightforward to restore this behaviour.

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 checked the behaviour on main. Here are some recordings:

tri-state-before

When starting from no selections and adding, it appears indeterminate:

tri-state-before-2

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 played around with this for a while and am can't tell why it wouldn't work. Sticking a breakpoint in the property getter the CheckState value is bound to shows it's getting a null but displaying as a false. Anyone got any thoughts or ideas here?

Note that this is not yet working correcetly due to an apparent bug in the control being used. That's being investigated separately.
src/Aspire.Dashboard/Components/Pages/Resources.razor Outdated Show resolved Hide resolved
<FluentPopover AnchorId="typeFilterButton" @bind-Open="_isTypeFilterVisible">
<Header>Resource Types</Header>
<Body>
<FluentStack Orientation="Orientation.Vertical">
<FluentCheckbox Label="All" CheckStateChanged="HandleTypeFilterShowAllChanged"
Copy link
Member

Choose a reason for hiding this comment

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

The current code doesn't seem to respond to clicking the All checkbox as expected, but I think its because of the ShowIndeterminate parameter in my other comment and not because of the changes from CheckState/CheckStateChanged to @bind-CheckState. I think when it is checked and you click it, it goes to indeterminate state, but then immediately gets flipped back to true because nothing changed with the other checkboxes.

@drewnoakes
Copy link
Member Author

GIF of latest revision's behaviour:

after

@tlmii
Copy link
Member

tlmii commented Dec 6, 2023

GIF of latest revision's behaviour:

For whenever someone is looking through the history when trying to fix the indeterminate issue, I'll note that @drewnoakes captured the timing issue working correctly once in this .gif - when unchecking Executable near the beginning of the animation, All shows the indeterminate state. But all other times unchecking things it does not.

@drewnoakes drewnoakes merged commit 2b15a50 into dotnet:main Dec 7, 2023
8 checks passed
@drewnoakes drewnoakes deleted the dynamic-resource-types branch December 7, 2023 00:22
@github-actions github-actions bot locked and limited conversation to collaborators Apr 27, 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.

2 participants