-
Notifications
You must be signed in to change notification settings - Fork 675
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
Show Package Owner profile links on Browse Tab #5766
Conversation
3139c76
to
a4fd5f2
Compare
aab2eba
to
c9f0c79
Compare
c9f0c79
to
342535f
Compare
_link = knownOwner.Link; | ||
} | ||
|
||
public string Name => _name; |
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.
public string Name {get;} is syntactic sugar over having a private and public field.
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.
True, I created a field at some point during development, but it's not really necessary, I'll revise that.
public Uri? ProjectUrl { get; internal set; } | ||
public DateTimeOffset? Published { get; internal set; } | ||
public IReadOnlyList<string>? OwnersList { get; internal set; } | ||
public string? Owners { get; internal set; } | ||
public IReadOnlyList<KnownOwner>? KnownOwners |
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.
This class is going from a model DTO (post discussion), to something that has action logic in addition to being a model.
i don't have a clear recommendation, but something that stood out.
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.
Talked offline. This is kind of a DTO versus Model, so I'll move the calculation logic down into SearchObject
.
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.
Bug
Fixes: NuGet/Home#12501
Spec: https://github.com/NuGet/Home/blob/dev/accepted/2023/owner-author-pmui.md
Regression? Last working version:
Description
Key points
On the Browse Tab, when a single package source is selected and it provides a resource,
OwnerDetailsUriTemplateResourceV3
, hyperlinks will be shown for each package owner in the PM UI Packages List. This is referred to as Supporting Known Owners.The owner profile URLs are built from the template from that source and the link's ToolTip shows the full URL.
No telemetry is added in this PR.
Screenshot of single package source Supporting Known Owners:
Authors will never be shown in the packages list when the source Supports Known Owners.
In all other scenarios, Author will continue to be shown when Known Owners is not supported.
Screenshot of single package source which does not Support Known Owners:
Commentary
IOwnerDetailsUriService
, provides an indicator for support for known owners, and a means to obtain an Owner Profile URL.MultiSourcePackageMetadataProvider
implements and provides this service:PackageSearchMetadataContextInfo
is the coming together of two things: Search results and package metadata. Owners belong to Search, therefore this context is where I'm materializingKnownOwner
models.author
" when there's no Supported Known Owners.owner
" when there are Supported Known Owners.KnownOwnerViewModels
, as they should match. I kept this pattern here as I'd like to eventually refactor this class and use a more MVVM approach for the AuthorAndDownloadCount's view model.ExecuteOpenExternalLink
event similar to the Details Pane event. Telemetry can be added for owner profile links by simply providing a new property which this event will emit.KnownOwnerViewModel
represents known owners within Visual Studio UIs.LastItemToVisibilityConverter
allows for adding commas after each hyperlink, except the last one.Accessibility
PR Checklist
PR has a meaningful title
PR has a linked issue.
Described changes
Tests
Documentation