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

Resource Command Support #2572

Merged
merged 5 commits into from
Mar 5, 2024
Merged

Resource Command Support #2572

merged 5 commits into from
Mar 5, 2024

Conversation

tlmii
Copy link
Member

@tlmii tlmii commented Mar 1, 2024

First pieces of the resource command support:

  • Forked the FluentMenuButton component to make an AspireMenuButton component that had a few extra features (different item binding/commanding structure, ability to change the appearance of the button, ability to have only an icon of our choosing for the button). Already chatting with @vnbaaij about what to do long term
  • Temporarily implemented commands in the local resource server. I won't merge until that is removed, but it'll make it easier for folks to test it out. Start/Stop commands always fail. Restart always succeeds. Start/Restart have confirmation message, Stop does not. Some resources (randomly) have no commands at all.
  • If no commands are present on a particular resource, the button is still there, but its disabled
  • If no commands are present on any resources, the column doesn't appear at all
  • Toast component pops up for Success/Failure of the command. Failure has a View Logs link to take you to the console logs for the resource. Not 100% positive that's useful, but wanted to show it for discussion
  • MessageBox component used for confirmation

Here's a quick recording of it in action:

ResourceCommandsDark

Some notes:

  1. I had to hack around a glitch where the menu would appear offscreen very briefly, causing the horizontal scrollbar to appear. That's what the extra CSS for the grid does. This is only an issue when its the last column (or otherwise near the right edge of the screen)
  2. I initially had no text in the column header for the commands. I didn't think it was necessary. But there's an odd rendering issue with the grid where the resize handle doesn't appear if there's no text. So I just made it say Commands
  3. I have some special casing in the error handling for when ExecuteResourceCommand isn't implemented by the resource server. I'm not sure that's really necessary, but it seemed appropriate at this state where we are just starting to implement that (and indeed it is not implemented in the local server once I remove my change)
  4. As mentioned above, I'll remove the changes to Hosting's DashboardService.cs and Partials.cs before this gets merged.
  5. The toasts disappear after 7s (the default). We should consider whether that should be increased/decreased. We should also make sure it plays well with screen readers (if it doesn't, that should probably be fixed in the component long term).
  6. We should consider if there needs to be keyboard shortcuts for the toasts.
  7. I did not implement merging commands that exist on resource types into the ones that exist on a resource as part of this PR. At the moment, we don't really use the resource type object from the API in a way that made that straightforward. Since the first use case for these commands doesn't use it either, I think we can do that as a second step.
Microsoft Reviewers: Open in CodeFlow

ArgumentException.ThrowIfNullOrWhiteSpace(displayName);

CommandType = commandType;
DisplayName = displayName;
Copy link
Member

Choose a reason for hiding this comment

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

Assuming we would want to localize the displayName?

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 assume we will want that localized somehow, along with things like custom statuses and whatnot. But I'm not sure what the plan is for that. Will discuss with folks, but I think that can come in a separate PR.

Copy link
Member

Choose a reason for hiding this comment

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

We should consider sending user culture to hosting and letting hosting localize

@davidfowl
Copy link
Member

Very cool @tlmii. I'm planning to look at implementing restart in Aspire.Hosting. The change looks like I would expect! We should think about future operations that may need more than a confirmation and make sure the resource server contract can expand to it.

@tlmii
Copy link
Member Author

tlmii commented Mar 4, 2024

Very cool @tlmii. I'm planning to look at implementing restart in Aspire.Hosting. The change looks like I would expect!

Nice - let me know what you think of the experience when you start that - I'm expecting that'll generate more feedback since it'll widen the audience for this UX.

We should think about future operations that may need more than a confirmation and make sure the resource server contract can expand to it.

Yeah I think this area will need more attention as we get a better idea of the commands that need to run through it. Even now with the confirmation dialog, it feels a bit forced. For example, what should the buttons say? Yes/No? OK/Cancel? Whatever we pick forces the language that can be used in the confirmation dialog. So even that could be more flexible - though we probably don't want to go the route of implementing an entire MessageBox API in the resource server api.

@tlmii
Copy link
Member Author

tlmii commented Mar 4, 2024

I went ahead and removed the test code. If anyone wants to run it, they can grab the 2nd to last commit (30e7655 as of this writing) to make testing easier - or just use the code in the original commit (c1a1262#diff-53cce2ea4b16776f87f7628686480c626c9fce25ea5a28eb46dd95f1dc72717f and c1a1262#diff-d35f6c3b9acebacf4a61b2055f987e92d1a207f889ac9892a6c4ad92f6492186) as a template for your own.

@tlmii tlmii marked this pull request as ready for review March 4, 2024 05:45
@JamesNK
Copy link
Member

JamesNK commented Mar 4, 2024

I ran TestShop on the branch and didn't see the column. I'm guessing that means there are no commands.

How do we test this?

@tlmii
Copy link
Member Author

tlmii commented Mar 4, 2024

@JamesNK see my previous comment - you can pull the commit prior to the latest to get the test code I used or just use the small amount of test code from the first commit in the PR manually (the two files in Aspire.Hosting that have changes).

@davidfowl
Copy link
Member

Lets get this merged. I need to design the app model API for exposing this 😄

Copy link
Member

@JamesNK JamesNK left a comment

Choose a reason for hiding this comment

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

Feedback:

  • The ... button disable state is very subtle. It needs to be more obvious. Instead of changing the opacity, maybe the color should be grey.
  • The menu goes off the bottom of the screen:
    image
    Fixed an issue like this in metrics. Does the control you're using have the same setting? Update Blazor Fluent UI package to 4.4.2 preview #2301

Comments + issues here should be fixed but I trust you to do it in a follow up. Can merge now to unblock people

@tlmii
Copy link
Member Author

tlmii commented Mar 5, 2024

Thanks @JamesNK. I fixed a few of the things real quick before merge. Will tackle the rest in the morning in a separate PR.

@JamesNK
Copy link
Member

JamesNK commented Mar 5, 2024

Need to rebase. _clientCancellationToken is from a change yesterday.

@tlmii
Copy link
Member Author

tlmii commented Mar 5, 2024

@JamesNK

The ... button disable state is very subtle. It needs to be more obvious. Instead of changing the opacity, maybe the color should be grey.

Yeah, I'm not real fond of it but it's how the built-in control appears to work. I wanted to look at that specifically to see if there was something built-in to handle it or if we needed to go custom with it.

@tlmii tlmii enabled auto-merge (squash) March 5, 2024 07:13
@tlmii tlmii merged commit 58528a6 into dotnet:main Mar 5, 2024
8 checks passed
@tlmii tlmii deleted the dev/ResourceCommands branch March 5, 2024 07:33
@tlmii
Copy link
Member Author

tlmii commented Mar 6, 2024

Filed #2661 to track microsoft/fluentui-blazor#1640 to address this.

@tlmii
Copy link
Member Author

tlmii commented Mar 6, 2024

@JamesNK

The ... button disable state is very subtle. It needs to be more obvious. Instead of changing the opacity, maybe the color should be grey.

Yeah, I'm not real fond of it but it's how the built-in control appears to work. I wanted to look at that specifically to see if there was something built-in to handle it or if we needed to go custom with it.

Figured it out - PR here: #2670

@github-actions github-actions bot locked and limited conversation to collaborators Apr 20, 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.

5 participants