PM UI surfaces a 'View README' link to the Embedded Readme file by advay26-zz · Pull Request #3583 · NuGet/NuGet.Client

advay26-zz · 2020-08-14T22:23:47Z

Bug

Fix

Details: The PM UI's package metadata page now includes a 'View README' link if the user searches for a package on an HTTP source. Local sources do not currently surface this link, as that would involve parsing markdown to display the README file locally.

For an idea of what this looks like:

Testing/Validation

Tests Added: No
Reason for not adding tests: All the work was UI-related
Validation:

…ctUrl to see an an actual link for now

nkolev92 · 2020-08-19T22:18:14Z

        PackageIdentity Identity { get; }
        Uri LicenseUrl { get; }
        Uri ProjectUrl { get; }
+        Uri ReadmeUrl { get; }


If the plan is too soon/eventually have it displayed from local feed, an approach such as LicenseMetadata might be preferred.

Given @advay26's limited time left, what'd be the priority of this change?
@chgill-MSFT @zkat

nkolev92

Looks reasonable from my end.

I have a question about the API changes given that we'll likely want to surface readme information in the PM UI from local feeds/global packages folder.

donnie-msft

🚀

zkat · 2020-08-21T00:13:23Z

We should get in touch with PM to see if/when we plan on actually displaying READMEs from local feeds, but otherwise all looks good to me. 👍🏻

loic-sharma · 2020-08-21T17:18:55Z

    <value>View License</value>
  </data>
+  <data name="Text_ViewReadme" xml:space="preserve">
+    <value>View README</value>


The "README: View README" looks a tiny bit repetitive to me. Did y'all consider shortening it to "README: View"?

We did this to follow the "License: View License" precedent, but I'm happy to reconsider this with @chgill-MSFT if that feels like something that can be improved.

@loic-sharma I completely see what you're saying. It sticks out even more than View License because it's entirely capitalized 🤔 it also has the issue of users often thinking that it'll open locally rather than open in web.

The available potential options I see are:

"View README" (current)

"View" (less repetitive but also not clear where it'll open, also misaligned with License)

"View on NuGet.org" (clearer, but misaligned with License)

full URL to README (clearest, but ugly)

Any options I'm missing or alternative ideas? I can bring it up to the design office hours today 😊 Currently I'm leaning toward View on NuGet.org.

@jcjiang do you have any thoughts?

Note: The misalignment with embedded License issue can be fixed by easily by also changing how license is displayed. That being said, 2 adjacent lines that say "View on NuGet.org" might look weird?

With the "View on nuget.org" message, how would you know it's nuget.org? When the readme is on a different server, how would be generate the name to use in "view on (name)"?

Seems to me it'd be better to keep this a static wording.
Our other links show the full URL.... I'm not sure how great that would be, but we are doing it in 2 other links, there.
Have we/should we run this by design team?

it also has the issue of users often thinking that it'll open locally rather than open in web.

Our other links show the full URL.... I'm not sure how great that would be, but we are doing it in 2 other links, there.

These are good points that makes me lean towards showing the full URL. Do we know what the URL would be? Something like https://www.nuget.org/packages/newtonsoft.json/12.0.3/readme would seem pretty clean to me.

It'll look somewhat similar to that. We tested it with this URL: https://dev.nugettest.org/packages/embeddedreadmefiletest2/3.0.0/#show-readme-container (will end up being www.nuget.org instead of dev.nuggettest.org) but I imagine this can be cleaned up further.

* Made initial changes for PM UI work for Readmes * Added ReadmeUrl element to all the relevant places, still using ProjectUrl to see an an actual link for now * URL now shows when user hovers over the link's text * Removed the placeholder URL value that used the ProjectUrl

* Basic pack functionality for Embedded Readmes (#3575) * Made basic changes to classes to allow for a Readme element in the nuspec file * More changes for readme functionality * Adding the readme property if 'readme.md' is present in the base directory * Added support for dotnet pack for .csproj files * Added validation for Readmes when packing, brought dotnet pack up to speed with nuget pack * Changed variable names, changed error message for empty Readme * Removed commented code * Added a summary for the CheckForReadme method * Added support for dotnet pack for .csproj files * Added support for dotnet pack for .csproj files * Added validation for Readmes when packing, brought dotnet pack up to speed with nuget pack * Changed variable names, changed error message for empty Readme * Updated PublicAPI.Unshipped.txt files * Changed some spacing * Wrote tests for packing embedded readmes * Removed automatic packing of readmes from base directory * Used platform independent directory separating chars for testing * Update src/NuGet.Core/NuGet.Packaging/PackageCreation/Authoring/ManifestMetadata.cs Co-authored-by: donnie-msft <[email protected]> * Updated PublicAPI files after file structure was modified * Reverted to 'Readme == string.Empty' so that the Readme element remains optional * Removed 1 MB file size check on pack * Updated Public API files * Updated Public API files again, builds fine locally * Fixed a bug with dir. separating chars (which seemed to affect /* searches exclusively?), added some tests to catch it out * Changed var name in one of the tests * Changed tests that were constructed incorrectly for non-Windows platforms * Added dotnet pack tests Co-authored-by: donnie-msft <[email protected]> * PM UI surfaces a 'View README' link to the Embedded Readme file (#3583) * Made initial changes for PM UI work for Readmes * Added ReadmeUrl element to all the relevant places, still using ProjectUrl to see an an actual link for now * URL now shows when user hovers over the link's text * Removed the placeholder URL value that used the ProjectUrl Co-authored-by: Advay Tandon <[email protected]> Co-authored-by: donnie-msft <[email protected]>

advay26 added 4 commits August 7, 2020 13:14

Made initial changes for PM UI work for Readmes

be748c0

Added ReadmeUrl element to all the relevant places, still using Proje…

f6df1a4

…ctUrl to see an an actual link for now

URL now shows when user hovers over the link's text

3eeec2c

Removed the placeholder URL value that used the ProjectUrl

55ea2ea

advay26-zz requested review from aortiz-msft, cristinamanum, dominoFire, donnie-msft, dtivel, heng-liu, kartheekp-ms, nkolev92, rrelyea, zivkan and zkat as code owners August 14, 2020 22:23

zkat approved these changes Aug 18, 2020

View reviewed changes

nkolev92 reviewed Aug 19, 2020

View reviewed changes

donnie-msft approved these changes Aug 20, 2020

View reviewed changes

zkat approved these changes Aug 21, 2020

View reviewed changes

loic-sharma reviewed Aug 21, 2020

View reviewed changes

campersau mentioned this pull request Aug 26, 2020

Support Embedded Readme NuGetPackageExplorer/NuGetPackageExplorer#1095

Closed

2 tasks

zkat merged this pull request into dev-feature-readme Sep 2, 2020

zkat deleted the dev-advay26-readme-pmui branch September 2, 2020 23:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PM UI surfaces a 'View README' link to the Embedded Readme file#3583

PM UI surfaces a 'View README' link to the Embedded Readme file#3583
zkat merged 4 commits intodev-feature-readmefrom
dev-advay26-readme-pmui

advay26-zz commented Aug 14, 2020 •

edited

Loading

Uh oh!

nkolev92 Aug 19, 2020

Uh oh!

nkolev92 left a comment

Uh oh!

donnie-msft left a comment

Uh oh!

zkat commented Aug 21, 2020

Uh oh!

loic-sharma Aug 21, 2020 •

edited

Loading

Uh oh!

advay26-zz Aug 21, 2020

Uh oh!

chrisraygill Aug 21, 2020 •

edited

Loading

Uh oh!

zivkan Aug 21, 2020 •

edited

Loading

Uh oh!

donnie-msft Aug 24, 2020

Uh oh!

loic-sharma Aug 24, 2020

Uh oh!

advay26-zz Aug 24, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

Conversation

advay26-zz commented Aug 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Bug

Fix

Testing/Validation

Uh oh!

nkolev92 Aug 19, 2020

Choose a reason for hiding this comment

Uh oh!

nkolev92 left a comment

Choose a reason for hiding this comment

Uh oh!

donnie-msft left a comment

Choose a reason for hiding this comment

Uh oh!

zkat commented Aug 21, 2020

Uh oh!

loic-sharma Aug 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

advay26-zz Aug 21, 2020

Choose a reason for hiding this comment

Uh oh!

chrisraygill Aug 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zivkan Aug 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

donnie-msft Aug 24, 2020

Choose a reason for hiding this comment

Uh oh!

loic-sharma Aug 24, 2020

Choose a reason for hiding this comment

Uh oh!

advay26-zz Aug 24, 2020

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

advay26-zz commented Aug 14, 2020 •

edited

Loading

loic-sharma Aug 21, 2020 •

edited

Loading

chrisraygill Aug 21, 2020 •

edited

Loading

zivkan Aug 21, 2020 •

edited

Loading