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

Update ProjectInstallationInfo ToString() on Details Pane table and improve localization of table headers #6051

Conversation

martinrrm
Copy link
Contributor

@martinrrm martinrrm commented Sep 24, 2024

Bug

Fixes: NuGet/Home#13805

Description

Narrator wasn't reading the whole table, we use the property ToString() in PackageInstallationInfo for that, now Narrator reads the vulnerability and the package level

image
image

PR Checklist

  • Meaningful title, helpful description and a linked NuGet/Home issue
    - [ ] Added tests Narrator tested
  • Link to an issue or pull request to update docs if this PR changes settings, environment variables, new feature, etc.

@martinrrm martinrrm requested a review from a team as a code owner September 24, 2024 20:10
@martinrrm martinrrm changed the title Update narrator on Details Pane table Update narrator on Details Pane table and improve localization Sep 24, 2024
Copy link
Contributor

@jebriede jebriede left a comment

Choose a reason for hiding this comment

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

The max severity in the table should be localized.

The title, which becomes the commit messages, should state we're updating the ToString for accessibility. It's not quite accurate that we're updating Narrator for the details pane, we're actually updating the ToString so that any screen reader (including Narrator) will read the details correctly.

var vulnerabilityString = string.Empty;
if (InstalledVersionMaxVulnerability != -1)
{
vulnerabilityString = string.Format(CultureInfo.CurrentCulture, Resources.Label_PackageVulnerableToolTip, (PackageVulnerabilitySeverity)InstalledVersionMaxVulnerability);
Copy link
Contributor

Choose a reason for hiding this comment

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

The InstalledVersionMaxVulnerability enum values are not localized. We use IntToVulnerabilitySeverityConverter in the XAML to convert to a localized string. We should consider localization for this accessibility fix.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If im not wrong when we use CultureInfo it localizes the string.

I updated the code to use the converter, not sure if this is the best way to do it. We could also copy the code from the converter into this file.

@martinrrm martinrrm requested a review from jebriede September 25, 2024 04:18
@martinrrm martinrrm changed the title Update narrator on Details Pane table and improve localization Update ProjectInstallationInfo ToString() on Details Pane table and improve localization of table headers Sep 25, 2024
@jebriede jebriede merged commit aae9f6d into feature-TransitivePkgsInSolutionPMUI Sep 25, 2024
28 checks passed
@jebriede jebriede deleted the dev-martinrrm-narrator-solution-packagelevel branch September 25, 2024 18:23
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.

2 participants