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 changing display language in dashboard settings #6805

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

Conversation

adamint
Copy link
Member

@adamint adamint commented Nov 26, 2024

Description

Currently, you can change the language of the Aspire dashboard by changing the language of the browser. This change adds a setting in the settings dialog to change the display language. Formatting is not affected, as CultureInfo.CurrentCulture is unchanged. Only CultureInfo.CurrentUICulture is changed.

The one edge case is if there is a culture (such as es-US or pt-PT) that the user is requesting that is different than our localized culture (es and pt-BR respectively). The resolved cultures will be es and pt-BR, so we should select these as the active cultures in the dropdown - except in zh, since there are two different zh cultures that we localize for.

The approach recommended by the Blazor globalization docs was taken.

Animation

Fixes #5009

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 changed the title Dev/adamint/allow setting language in dashboard Allow changing display language in dashboard settings Nov 26, 2024
Immediate="true"
Label="@Loc[nameof(Dialogs.SettingsDialogLanguage)]"
Items="@_languageOptions"
OptionText="@(c => c.NativeName.Humanize())"
Copy link
Member

Choose a reason for hiding this comment

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

Do we know the default language for the browser? Having English (default) in the list seems like it would be useful.

Copy link
Member Author

@adamint adamint Nov 26, 2024

Choose a reason for hiding this comment

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

We could determine this based on the Accept-Language header or whether UI culture differs from culture, but CultureInfo.CurrentUICulture would just be set to english so that option would be selected, and the user would visibly see that it's english so it would seem a little redundant to me. What do you think?

Comment on lines +83 to +85
NavigationManager.NavigateTo(
DashboardUrls.SetLanguageUrl(_selectedUiCulture.Name, uri),
forceLoad: true);
Copy link
Member

@JamesNK JamesNK Nov 26, 2024

Choose a reason for hiding this comment

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

What is the reason to use a redirect to change the language? Was it related to setting the cookie? Or was it updating all of the text to a new language?

The problem with using a redirect is it's inconsistent with changing the theme. That happens without reloading the page, while language change reloads the page and loses state (e.g. the settings dialog itself immediately closes).

Did you consider setting the cookie in the browser with JS interop like changing the theme? That plus triggering a StateHasChanged on the overall page to rerender everything seems like it could work. Although I wouldn't be surprised if changing the culture in the middle of a HTTP request is problematic.

Copy link
Member Author

@adamint adamint Nov 27, 2024

Choose a reason for hiding this comment

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

Setting the cookie isn't the problem, updating text is unfortunately the real issue.

Did you consider setting the cookie in the browser with JS interop like changing the theme? That plus triggering a StateHasChanged on the overall page to rerender everything seems like it could work. Although I wouldn't be surprised if changing the culture in the middle of a HTTP request is problematic.

I did, but this approach doesn't work. Even after CultureInfo.CurrentUICulture is updated and localizers start returning the strings for the right language, StateHasChanged just doesn't actually update the page text. I've read more into this the past day, and have not been able to get any of these workarounds to work, so I'd strongly prefer sticking with the current approach - it's the recommended approach.

Copy link
Member

Choose a reason for hiding this comment

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

Those workarounds are specific to Blazor WASM which we're not using.

Copy link
Member Author

@adamint adamint Nov 27, 2024

Choose a reason for hiding this comment

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

Yes, those are the only workarounds I have found

Copy link
Member

@JamesNK JamesNK Nov 27, 2024

Choose a reason for hiding this comment

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

I'll take a look and see whether I can find a way to make it work.

If a refresh is required then I think a confirmation should be displayed after changing the language, e.g.

The dashboard must be refreshed when the language is changed. Are you sure you want to change the language?

Yes, No

Copy link
Member Author

Choose a reason for hiding this comment

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

If a refresh is required then I think a confirmation should be displayed after changing the language, e.g.

I don’t think this would be very useful. The user is pretty clear in their intent, the dialog adds unnecessary friction. Reloading (or opening a new page to select language in) is a common pattern, including on Microsoft websites

Copy link
Member

@JamesNK JamesNK Nov 27, 2024

Choose a reason for hiding this comment

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

But it’s inconsistent with the rest of the dashboard. Nothing else (adding filters, changing the theme, etc) triggers the dashboard to completely reload. Some page state could be lost.

There needs to be some notification that the page will reload. I think showing a yes/no dialog would be best, but a lower effort change could be to add some small text below the dropdown that changing the language will reload the dashboard.

@adamint adamint requested a review from JamesNK November 27, 2024 21:17
@inject IStringLocalizer<Dialogs> Loc

<FluentDialogBody>
<FluentStack Orientation="Orientation.Vertical" Style="display: flex; height: 100%" VerticalGap="0">
<FluentStack Orientation="Orientation.Vertical" Style="display: flex; height: 100%" VerticalGap="5">
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<FluentStack Orientation="Orientation.Vertical" Style="display: flex; height: 100%" VerticalGap="5">
<FluentStack Orientation="Orientation.Vertical" Style="display: flex; height: 100%" VerticalGap="8">

@inject IStringLocalizer<Dialogs> Loc

<FluentDialogBody>
<FluentStack Orientation="Orientation.Vertical" Style="display: flex; height: 100%" VerticalGap="0">
<FluentStack Orientation="Orientation.Vertical" Style="display: flex; height: 100%" VerticalGap="5">
<FluentRadioGroup TValue="string"
Copy link
Member

Choose a reason for hiding this comment

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

Place in <div>. Otherwise the label and content have gap inserted between them from stack

Orientation="Orientation.Vertical">
<FluentRadio Value="@ThemeManager.ThemeSettingSystem">@Loc[nameof(Dialogs.SettingsDialogSystemTheme)]</FluentRadio>
<FluentRadio Value="@ThemeManager.ThemeSettingLight">@Loc[nameof(Dialogs.SettingsDialogLightTheme)]</FluentRadio>
<FluentRadio Value="@ThemeManager.ThemeSettingDark">@Loc[nameof(Dialogs.SettingsDialogDarkTheme)]</FluentRadio>
</FluentRadioGroup>

<FluentSelect
Copy link
Member

Choose a reason for hiding this comment

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

Place in <div>. Otherwise the label and content have gap inserted between them from stack

httpContext.Response.Cookies.Append(
CookieRequestCultureProvider.DefaultCookieName,
// keep current culture for formatting, etc., but change ui (display) culture
CookieRequestCultureProvider.MakeCookieValue(new RequestCulture(CultureInfo.CurrentCulture.Name, language)));
Copy link
Member

Choose a reason for hiding this comment

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

Right now this is a session cookie and is lost when the browser is complete closed. Add an expiry:

new CookieOptions { Expires = DateTimeOffset.UtcNow.AddYears(1) }

Comment on lines +80 to +85
var uri = new Uri(NavigationManager.Uri)
.GetComponents(UriComponents.PathAndQuery, UriFormat.Unescaped);

NavigationManager.NavigateTo(
DashboardUrls.SetLanguageUrl(_selectedUiCulture.Name, uri),
forceLoad: true);
Copy link
Member

Choose a reason for hiding this comment

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

Add a comment here that a cookie must be set and the dashboard reloaded to use the new culture set by localization middle.

Comment on lines +35 to +37
if (!_languageOptions.Contains(CultureInfo.CurrentUICulture)
&& !StringComparers.Culture.Equals(CultureInfo.CurrentUICulture.TwoLetterISOLanguageName, "zh")
&& _languageOptions.FirstOrDefault(c => StringComparers.Culture.Equals(c.TwoLetterISOLanguageName, CultureInfo.CurrentUICulture.TwoLetterISOLanguageName)) is var matchedCulture)
Copy link
Member

Choose a reason for hiding this comment

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

This seems like it could be prone to mistakes. Tests are needed here. It could be moved to a class and have some unit tests that test different cultures.

I looked at what the middleware does and it uses Parent property on culture. What about using that to match?

https://github.com/dotnet/aspnetcore/blob/81a2bab8704d87d324039b42eb1bab0d977f25b8/src/Middleware/Localization/src/RequestLocalizationMiddleware.cs#L172

@@ -437,6 +438,19 @@ await Microsoft.AspNetCore.Authentication.AuthenticationHttpContextExtensions.Si
{
_app.MapPost("/authentication/logout", () => TypedResults.SignOut(authenticationSchemes: [CookieAuthenticationDefaults.AuthenticationScheme, OpenIdConnectDefaults.AuthenticationScheme]));
}

_app.MapGet("/api/set-language", (string? language, string redirectUrl, HttpContext httpContext) =>
Copy link
Member

Choose a reason for hiding this comment

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

Return BadRequest result if language or redirectUrl are null or empty.

@drewnoakes
Copy link
Member

My understanding is that CultureInfo.CurrentUICulture exists for the process. For server-side Blazor, does this mean that when one user sets a language, all users have that setting pushed on them?

That might not be such a problem in the local development case, but for hosted environments like ACA, it might be problematic. If an issue, can we fix it, or allow this feature to be suppressed in such cases?

@JamesNK
Copy link
Member

JamesNK commented Nov 28, 2024

CultureInfo.CurrentUICulture is thread local.

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.

Let me change language of dashboard
3 participants