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.

Sign up for GitHub

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

Jump to bottom

Update display name and description for built-in lifecycle commands #7334

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

Update display name and description for built-in lifecycle commands #7334

wants to merge 4 commits into from

Conversation

adamint
Copy link
Member

@adamint adamint commented Jan 30, 2025 •
edited
Loading

Description

Fixes #6982 - sets the display name and description for the three lifecycle commands in the resource service
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
  • 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?

@adamint adamint requested review from JamesNK and drewnoakes January 30, 2025 21:49
@danmoseley danmoseley requested a review from Copilot February 3, 2025 21:07
Copilot
Copilot AI reviewed Feb 3, 2025
View reviewed changes

Choose a reason for hiding this comment

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

Copilot reviewed 1 out of 17 changed files in this pull request and generated no comments.

Files not reviewed (16)
  • src/Aspire.Dashboard/Aspire.Dashboard.csproj: Language not supported
  • src/Aspire.Dashboard/Resources/Commands.Designer.cs: Language not supported
  • src/Aspire.Dashboard/Resources/Commands.resx: Language not supported
  • src/Aspire.Dashboard/Resources/xlf/Commands.cs.xlf: Language not supported
  • src/Aspire.Dashboard/Resources/xlf/Commands.de.xlf: Language not supported
  • src/Aspire.Dashboard/Resources/xlf/Commands.es.xlf: Language not supported
  • src/Aspire.Dashboard/Resources/xlf/Commands.fr.xlf: Language not supported
  • src/Aspire.Dashboard/Resources/xlf/Commands.it.xlf: Language not supported
  • src/Aspire.Dashboard/Resources/xlf/Commands.ja.xlf: Language not supported
  • src/Aspire.Dashboard/Resources/xlf/Commands.ko.xlf: Language not supported
  • src/Aspire.Dashboard/Resources/xlf/Commands.pl.xlf: Language not supported
  • src/Aspire.Dashboard/Resources/xlf/Commands.pt-BR.xlf: Language not supported
  • src/Aspire.Dashboard/Resources/xlf/Commands.ru.xlf: Language not supported
  • src/Aspire.Dashboard/Resources/xlf/Commands.tr.xlf: Language not supported
  • src/Aspire.Dashboard/Resources/xlf/Commands.zh-Hans.xlf: Language not supported
  • src/Aspire.Dashboard/Resources/xlf/Commands.zh-Hant.xlf: Language not supported
drewnoakes
drewnoakes reviewed Feb 5, 2025
View reviewed changes
src/Aspire.Dashboard/ResourceService/Partials.cs
.ToImmutableArray();

// Use custom localizations for built-in lifecycle commands
static (string DisplayName, string DisplayDescription) GetDisplayNameAndDescription(string commandName, string displayName, string description)
Copy link
Member

Choose a reason for hiding this comment

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

I thought we were going to add localized values to the proto, so that other resource services (like that in ACA) can also have localized command strings.

Copy link
Member

Choose a reason for hiding this comment

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

I see James suggested this approach in the linked issue. I don't think it'd be too bad to add support for this to the proto. I would expect ACA to have loc requirements. cc @snehapar9

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 temporary solution for 9.1, we can improve this later @drewnoakes

@drewnoakes
Copy link
Member

Fixes #6982 - sets the display name and description for the three lifecycle commands in the resource service

This PR doesn't address all the strings in #6982. Were the others addressed elsewhere?

@adamint
Copy link
Member Author

adamint commented Feb 5, 2025

Fixes #6982 - sets the display name and description for the three lifecycle commands in the resource service

This PR doesn't address all the strings in #6982. Were the others addressed elsewhere?

I was planning on opening a separate PR for those strings. Cherry picked the commit into this branch

JamesNK
JamesNK approved these changes Feb 7, 2025
View reviewed changes
src/Aspire.Dashboard/Components/Controls/ExceptionDetails.razor
@@ -30,6 +29,6 @@

private void OnExceptionDetailsClicked(MouseEventArgs e)
{
_ = TextVisualizerDialog.OpenDialogAsync(ViewportInformation, DialogService, Loc[nameof(ControlsStrings.ExceptionDetailsTitle)], ExceptionText);
_ = TextVisualizerDialog.OpenDialogAsync(ViewportInformation, DialogService, DialogsLoc[nameof(Dashboard.Resources.Dialogs.DialogCloseButtonText)], Loc[nameof(ControlsStrings.ExceptionDetailsTitle)], ExceptionText);
Copy link
Member

Choose a reason for hiding this comment

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

Pass the DialogsLoc into the method and get the text from it inside the method. Removes some duplication.

src/Aspire.Dashboard/ResourceService/Partials.cs
Comment on lines +121 to +140
const string startCommandName = "resource-start";
const string stopCommandName = "resource-stop";
const string restartCommandName = "resource-restart";

if (commandName is startCommandName)
{
return (CommandsResources.StartCommandDisplayName, CommandsResources.StartCommandDisplayDescription);
}

if (commandName is stopCommandName)
{
return (CommandsResources.StopCommandDisplayName, CommandsResources.StopCommandDisplayDescription);
}

if (commandName is restartCommandName)
{
return (CommandsResources.RestartCommandDisplayName, CommandsResources.RestartCommandDisplayDescription);
}

return (displayName, description);
Copy link
Member

Choose a reason for hiding this comment

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

A switch expression would work well here.

JamesNK
JamesNK reviewed Feb 7, 2025
View reviewed changes
src/Aspire.Dashboard/ResourceService/Partials.cs
Comment on lines +121 to +123
const string startCommandName = "resource-start";
const string stopCommandName = "resource-stop";
const string restartCommandName = "resource-restart";
Copy link
Member

@JamesNK JamesNK Feb 7, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Add an internal KnownResourceCommands type in the shared folder and move the strings there (same pattern as KnownResourceTypes, etc).

KnownResourceCommands can be used here and in CommandsConfigurationExtensions. This prevents the command names from accidently going out of sync between app host and dashboard.

@JamesNK
Copy link
Member

JamesNK commented Feb 7, 2025

Good to merge once changes above are made.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JamesNK JamesNK JamesNK approved these changes

Copilot code review Copilot Copilot left review comments

@drewnoakes drewnoakes Awaiting requested review from drewnoakes

@davidfowl davidfowl Awaiting requested review from davidfowl

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[WebToolsE2E][Aspire] Some strings in the dashboard of Aspire 9.1 are not localized.
3 participants
@adamint @drewnoakes @JamesNK