-
Notifications
You must be signed in to change notification settings - Fork 641
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
Display NuGet Package Explorer link on details page. #8697
Conversation
<i class="ms-Icon ms-Icon--FabricFolderSearch" aria-hidden="true" aria-label="@disclaimer" title="@disclaimer"></i> | ||
<a href="@Model.NuGetPackageExplorerUrl" data-track="outbound-nugetpackageexplorer-url" | ||
aria-label="open in NuGet Package Explorer" | ||
title="Explore additional package info on NuGet Package Explorer" target="_blank" rel="nofollow"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we should be using target="_blank"
. This is different behavior than the project URL or repo URL which I don't see as categorically different. However I don't feel strongly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What was the motivation for this change on the FuGet link? Perhaps I am missing something
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With target="_blank"
we open a new browser tab with the Fuget/NPE page while keeping the current NuGet display package page. Since those links sends you to a 3rd party site, I think it's better to have our NuGet display package page open while the user go and see those sites for more information. Basically to avoid the user to go backwards on the browser history to return to the same NuGet display package page.
Looks good to me! Thanks for cleaning up the old, IE-only thing 👏. Wanna take a final look, @jcjiang + @clairernovotny? |
Summary of the changes:
DisplayPackage page:
DisplayPackageV2 page:
Addresses #8428