[Feature] Render package README from local storage in the PM UI #6110

jgonz120 · 2024-10-21T17:39:39Z

Bug

Fixes: https://github.com/NuGet/Client.Engineering/issues/2793

Description

Update the PM UI to render package README for packages found locally. To avoid confusion the README only renders in the Installed, Update, and Consolidate tabs. A new feature flag was added, NUGET_RENDER_README_IN_PMUI, that can be used to disable this feature. This was reviewed as a feature branch and the changes previously reviewed are listed below

Screenshots

Package has a README

Package has no README defined

Browse tab

PLOC

Accessibility Tests

Previous PRs

#6012
#6014
#6029
#6094

Rebase Changes

Due to conflicts the following files were changed during the rebase.
Conflicts:
.vsixinclude
.vsixignore
Directory.Packages.props
NuGet.Protocol\PublicApi\net472\PublicApi.Unshipped.txt
NuGet.Protocol\PublicApi\netstandard2.0\PublicApi.Unshipped.txt
NuGet.Protocol\PublicApi\net8.0\PublicApi.Unshipped.txt
Automerged:
PackageSearchMetadataContextInfo.cs

I also had to update RecommendedPackageSearchMetadata.cs to implement the ReadmeFileUrl property on the IPackageSearchMetadata interface.

PR Checklist

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

…6012) This package will be used for rendering markdown in the PM UI

Adds the ReadmeFileUrl field to IPackageSearchMetadata and implements the ability to get the URL for local packages. Future PRs will include implementations for getting the URL for remote READMEs and downloading them. --------- Co-authored-by: Andy Zivkovic <zivkan@users.noreply.github.com>

) Adds a new ViewModel for getting the README contents, leveraging the NuGetPackageFileService to get the file from the package. --------- Co-authored-by: Jean-Pierre Briedé <jebriede@microsoft.com>

…ils pane. (#6094) * Add ReadmeFileUrl field to IPackageSearchMetadata (#6014) Adds the ReadmeFileUrl field to IPackageSearchMetadata and implements the ability to get the URL for local packages. Future PRs will include implementations for getting the URL for remote READMEs and downloading them. --------- Co-authored-by: Andy Zivkovic <zivkan@users.noreply.github.com> * Added new user controls, updated details * Adjust readme viewmodel name * update views * Ensured ReadmefileUrl is set when merging sources * Fix delegation, update ux to render properly * Limit local readme rendering to excluse browse tab * ensure no readme found message for local packages. Added tests * update property count * lock url * fix rebase weirdness. * PR comments * Simplify * Move model to viewmodel * Renamed controls so they're less confusing * Switched to templates for tabs * fix typo * Update src/NuGet.Clients/NuGet.PackageManagement.VisualStudio/PackageFeeds/MultiSourcePackageMetadataProvider.cs Co-authored-by: Donnie Goodson <49205731+donnie-msft@users.noreply.github.com> * PR comment * rename * rename tests * change spacing to make pr easier to see * more spacing * Update src/NuGet.Clients/NuGet.PackageManagement.UI/ViewModels/PackageDetailsTabViewModel.cs Co-authored-by: Donnie Goodson <49205731+donnie-msft@users.noreply.github.com> * Revert "Update src/NuGet.Clients/NuGet.PackageManagement.UI/ViewModels/PackageDetailsTabViewModel.cs" This reverts commit add1b00. * IsVisible rename * add is visible * extra line * rename tsts * Reduce tab dependencies * pr comments * Adjust how selected tab is set * rename loaded/unloaded * avoid setting visibility, make tabs more generic * create conversion for vm to enum * pr comments * Update src/NuGet.Clients/NuGet.PackageManagement.UI/ViewModels/PackageDetailsTabViewModel.cs Co-authored-by: Jean-Pierre Briedé <jebriede@microsoft.com> --------- Co-authored-by: Andy Zivkovic <zivkan@users.noreply.github.com> Co-authored-by: Donnie Goodson <49205731+donnie-msft@users.noreply.github.com> Co-authored-by: Jean-Pierre Briedé <jebriede@microsoft.com>

donnie-msft

It's all coming together!

I know at one point we talked about PLOC and Accessibility testing. Have those been performed on the latest commits? Please add screenshots and comments about this in the description. Our UI Guidelines are here: https://github.com/NuGet/NuGet.Client/blob/dev/docs/ui-guidelines.md
Let me know if you have any questions about these processes.

src/NuGet.Clients/NuGet.VisualStudio.Client/.vsixinclude

donnie-msft · 2024-10-21T22:16:43Z

src/NuGet.Clients/NuGet.PackageManagement.UI/ViewModels/PackageDetailsTabViewModel.cs

+
+        public DetailControlModel DetailControlModel { get; private set; }
+
+        public ObservableCollection<TitledPageViewModelBase> Tabs { get; private set; }


I believe last week in your feature branch PR I suggested making this an IReadOnlyList. Can you help me understand why this collection needs to be dynamic?

Oh sorry, if I make the tab a ReadonlyList then I cannot make changes to the items in the tabs, they need to be instantiated in the constructor. Right now, I'm instantiating the VM in the constructor of the view, so I don't have all the components necessary to instantiate the VM. I think once we refactor the DetailControlModel it can handle creating the VM.

donnie-msft · 2024-10-21T22:19:52Z

src/NuGet.Clients/NuGet.PackageManagement.UI/Xamls/PackageManagerTopPanel.xaml

      </Grid.ColumnDefinitions>
-
+      


there are some locations where we do an extra line after the ColumnDefinitions, is there a style guide for this?

An extra line is ok but the extra spaces don't need to be there.

donnie-msft · 2024-10-21T22:23:24Z

src/NuGet.Clients/NuGet.PackageManagement.UI/Xamls/PackageManagerControl.xaml.cs

@@ -678,7 +686,8 @@ public void SaveSettings()
                FileConflictAction = _detailModel.Options.SelectedFileConflictAction.Action,
                IncludePrerelease = _topPanel.CheckboxPrerelease.IsChecked == true,
                SelectedFilter = _topPanel.Filter,
-                OptionsExpanded = _packageDetail._optionsControl.IsExpanded
+                OptionsExpanded = _packageDetail._optionsControl.IsExpanded,
+                SelectedPackageMetadataTab = PackageDetailsTabViewModel.ConvertFromTabType(_packageDetail._packageDetailsTabControl.PackageDetailsTabViewModel.SelectedTab)


I believe I mentioned this in previous PR reviews, but it's not clear why the PackageManagerControl needs to be directly involved with the details pane's tabs. There are multiple models and ViewModels between this control and the tab controls. Could you explain design at a high level so I know the reasoning behind this?

This is where the user settings are being saved. I need to get the current selected metadata tab in order to preserve the selected tab when the PM UI is opened again.

donnie-msft · 2024-10-21T22:36:04Z

src/NuGet.Clients/NuGet.PackageManagement.UI/Xamls/PackageManagerControl.xaml.cs

@@ -1269,6 +1278,7 @@ private void Filter_SelectionChanged(object sender, FilterChangedEventArgs e)

                NuGetUIThreadHelper.JoinableTaskFactory.RunAsync(async () =>
                {
+                    await _packageDetail._packageDetailsTabControl.PackageDetailsTabViewModel.SetCurrentFilterAsync(_topPanel.Filter);


I don't recall this change before but am curious about the design choice of passing the topPanel's tab to be stored and managed inside the PackageDetailsTabViewModel.
It's already available in the PackageManagerControl which is at the top of the hierarchy that creates these TabViewModels. It can just be passed with Control.ActiveFilter or with some refactoring it could be part of one of the parent ViewModels.

The current tab is used to determine if local readmes are being rendered. I switched it to use ActiveFilter, but I do think it's necessary for the ReadmePreviewViewModel to know what the current tab is so it can handle rendering. @jebriede and I discussed how this would be better met with an event that the VM would listen to and can update itself but that would require a refactoring that feels outside of the scope of this PR.

I don't recall this change before but am curious about the design choice of passing the topPanel's tab to be stored and managed inside the PackageDetailsTabViewModel. It's already available in the PackageManagerControl which is at the top of the hierarchy that creates these TabViewModels. It can just be passed with Control.ActiveFilter or with some refactoring it could be part of one of the parent ViewModels.

I agree with this idea. I also think it's a good idea not to manage a copy of the state of the top panel in a view model that pertains to something else. If the view model needs to react to a change in the top panel's selected tab (ideally through a state change in the model or view model, not directly responding to the UI change, although I know we don't currently have a VM for that panel that tracks this state), it can do that, but storing it requires it to manage the state of its internal representation of what the top panel's tab is and that's introducing the need to synchronize the state of that variable with whatever the top panel is.

Thinking about this for now I want to leave the control of the visibility of the Readme tab inside of ReadmePreviewViewModel.

I am changing the name of the function to ItemFilterChangedAsync which I think is more accurate to what we want. The VM doesn't need to store the current state it just needs to respond to when the filter changes. Since this view is the one that currently updates the filter on the DetailControlModel, I think it makes sense to let it also set the filter for the ReadmePreviewViewModel. Though I changed it so it fires the change directly on ReadmePreviewViewModel instead of a method inside of the PackageDetailsTabViewModel.

jebriede

Approving with suggestions

src/NuGet.Clients/NuGet.PackageManagement.UI/ViewModels/PackageDetailsTabViewModel.cs

src/NuGet.Clients/NuGet.PackageManagement.UI/Xamls/PackageMetadataControl.xaml.cs

src/NuGet.Clients/NuGet.PackageManagement.UI/Xamls/PackageReadmeControl.xaml.cs

src/NuGet.Core/NuGet.Protocol/Model/LocalPackageSearchMetadata.cs

src/NuGet.Clients/NuGet.PackageManagement.UI/ViewModels/PackageDetailsTabViewModel.cs

src/NuGet.Clients/NuGet.PackageManagement.UI/ViewModels/TitledPageViewModelBase.cs

donnie-msft

Thank you for adding the Accessibility FastPass results, PLOC testing, and the UI screenshots.

From the responses to my MVVM suggestions it seems like the plan is to iterate on that in upcoming previews. I agree with that approach to be MVVM-forward now and to continue improving it in future VS Previews.

I see 2 blockings issues even for Preview 1 at the moment:

The owner profile hyperlinks are no longer rendering in the Details pane, even in the Browse tab.
While the FastPass succeeded, my testing of Narrator shows that the 'Package Details' tab is not named properly. It should have an accessible name property like our other controls so that the default ToString isn't used by screen readers (eg, now screenreaders announce something like "PackageMetadata custom, (full namespace of the new tab control).

donnie-msft · 2024-10-22T23:01:51Z

src/NuGet.Clients/NuGet.PackageManagement.UI/Xamls/PackageManagerControl.xaml

-            </ColumnDefinition>
-            <ColumnDefinition Width="auto" />
-            <ColumnDefinition
+              Width="67*">


I'm only seeing a couple minor changes to values here, but this diff is large since there's so many whitespace changes. Please revert all the whitespace changes in this file.

donnie-msft

We've been working together offline and gotten both of the original issues I reported resolved. Thank you for working diligently on those issues!

There are 2 new issues:

JAWS is reading the entire contents of the Package Details pane when first visiting it. All labels are read for some reason (eg, "Description Version colon"). I've not seen this before so will take some time to advise on what's happening.
The focus border is being truncated on the package details contents

donnie-msft

Thanks again for the quick offline discussion and addressing the feedback.

I've decided that the remaining issues can be addressed in separate PRs since this is going into Preview 1 and there's ample time to address them.

These are the follow-ups:
NuGet/Home#13878
NuGet/Home#13879
NuGet/Home#13880

jgonz120 and others added 4 commits October 21, 2024 11:52

Add package reference for Microsoft.VisualStudio.Markdown.Platform (#…

97e555e

…6012) This package will be used for rendering markdown in the PM UI

Added the ReadmePreviewViewModel & edited NuGetPackageFileService (#6029

575b1e8

) Adds a new ViewModel for getting the README contents, leveraging the NuGetPackageFileService to get the file from the package. --------- Co-authored-by: Jean-Pierre Briedé <jebriede@microsoft.com>

jgonz120 force-pushed the dev-feature-ReadMeRenderingInPMUI branch from 8dba8d2 to 2c9fde3 Compare October 21, 2024 18:57

Rebase Conflicts

8ae18e1

jgonz120 changed the title ~~Dev feature read me rendering in pm UI~~ [Feature] Render package README from local storage in the PM UI Oct 21, 2024

jgonz120 marked this pull request as ready for review October 21, 2024 20:10

jgonz120 requested a review from a team as a code owner October 21, 2024 20:10

donnie-msft reviewed Oct 21, 2024

View reviewed changes

jgonz120 added 2 commits October 21, 2024 16:22

remove extra line

c9ba1fd

PR comments

0276fe3

jebriede previously approved these changes Oct 22, 2024

View reviewed changes

PR comments

9b29852

jgonz120 dismissed jebriede’s stale review via 9b29852 October 22, 2024 19:35

zivkan mentioned this pull request Oct 22, 2024

Add support for markdown > alerts via markdig NuGet/NuGetGallery#10173

Open

jebriede previously approved these changes Oct 22, 2024

View reviewed changes

nkolev92 previously approved these changes Oct 22, 2024

View reviewed changes

donnie-msft requested changes Oct 22, 2024

View reviewed changes

Fix owners links

fa91f76

jgonz120 dismissed stale reviews from nkolev92 and jebriede via fa91f76 October 23, 2024 01:04

jgonz120 added 6 commits October 22, 2024 19:26

Added logic to remove tab from list and fix tabname

9ef9ea7

fix spacing and narrator

d58a4d4

ran format

c349964

remove extra spacing

3ce17e3

fix keyboard navigation

5810517

finally fix focus 🥲

67bc6cb

donnie-msft reviewed Oct 23, 2024

View reviewed changes

donnie-msft approved these changes Oct 24, 2024

View reviewed changes

zivkan approved these changes Oct 29, 2024

View reviewed changes

donnie-msft merged commit dddfdd8 into dev Oct 29, 2024
30 of 32 checks passed

donnie-msft deleted the dev-feature-ReadMeRenderingInPMUI branch October 29, 2024 21:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Feature] Render package README from local storage in the PM UI #6110

[Feature] Render package README from local storage in the PM UI #6110

jgonz120 commented Oct 21, 2024 •

edited

Loading

donnie-msft left a comment

donnie-msft Oct 21, 2024

jgonz120 Oct 21, 2024

donnie-msft Oct 21, 2024

jgonz120 Oct 21, 2024

donnie-msft Oct 22, 2024

donnie-msft Oct 21, 2024

jgonz120 Oct 21, 2024

donnie-msft Oct 21, 2024

jgonz120 Oct 21, 2024

jebriede Oct 21, 2024 •

edited

Loading

jgonz120 Oct 22, 2024

jebriede left a comment

donnie-msft left a comment

donnie-msft Oct 22, 2024

donnie-msft left a comment

donnie-msft left a comment


		public DetailControlModel DetailControlModel { get; private set; }

		public ObservableCollection<TitledPageViewModelBase> Tabs { get; private set; }

[Feature] Render package README from local storage in the PM UI #6110

[Feature] Render package README from local storage in the PM UI #6110

Conversation

jgonz120 commented Oct 21, 2024 • edited Loading

Bug

Description

Screenshots

PLOC

Accessibility Tests

Previous PRs

Rebase Changes

PR Checklist

donnie-msft left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jebriede Oct 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jebriede left a comment

Choose a reason for hiding this comment

donnie-msft left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

donnie-msft left a comment

Choose a reason for hiding this comment

donnie-msft left a comment

Choose a reason for hiding this comment

jgonz120 commented Oct 21, 2024 •

edited

Loading

jebriede Oct 21, 2024 •

edited

Loading