[Prefix] List reserved namespaces in the manage packages page #4901
Conversation
shishirx34
requested review from
joelverhagen,
diverdan92,
ryuyu,
skofman1 and
scottbommarito
shishirx34
changed the title
| } | ||
|
|
||
| <div class="row user-package-list"> | ||
| <div class="col-md-12"> |
There was a problem hiding this comment.
looks like col-md-12 behaves correctly across different dimensions. Will leave as is.
| <a href="#" role="button" data-toggle="collapse" data-target="#reservednamespaces" | ||
| aria-expanded="true" aria-controls="reservednamespaces" id="show-reservednamespaces"> | ||
| <i class="ms-Icon ms-Icon--ChevronDown" aria-hidden="true"></i> | ||
| <span>My Reserved Package Ids</span> |
. Make sure to try it on various browsers (IE, Edge, Chrome, FF), various screen sizes (resize the window), and high contrast.
| public ReservedNamespaceViewModel(ICollection<ReservedNamespace> reservedNamespacesList) | ||
| { | ||
| ReservedNamespaces = reservedNamespacesList | ||
| .ToList() |
There was a problem hiding this comment.
The ToList should be after the Select, right?
There was a problem hiding this comment.
Don't need it actually, leftover from previous change. removed.
| <a target="_blank" href="@Url.User(owner.Username)">@owner.Username</a> | ||
| } | ||
| </td> | ||
| @if(reservedNamespace.IsPublic) |
There was a problem hiding this comment.
Add a space between @if and (
There was a problem hiding this comment.
formatted the document.
| <a href="#" role="button" data-toggle="collapse" data-target="#reservednamespaces" | ||
| aria-expanded="true" aria-controls="reservednamespaces" id="show-reservednamespaces"> | ||
| <i class="ms-Icon ms-Icon--ChevronDown" aria-hidden="true"></i> | ||
| <span>My Reserved Package Ids</span> |
There was a problem hiding this comment.
IMO I'd prefer
My Reserved Packages
or even
My Reserved Package Namespaces
I don't think we explicitly say IDs prominently in the UI anywhere else.
There was a problem hiding this comment.
This is as per @diverdan92 's spec. I think IDs clearly articulates the message here.
|
|
||
| namespace NuGetGallery | ||
| { | ||
| public class ReservedNamespaceViewModel |
There was a problem hiding this comment.
To follow the pattern of all of our other list view models this should be ReservedNamespaceListViewModel
| @@ -20,6 +20,10 @@ | |||
| } | |||
|
|
|||
| @{ | |||
| @Html.Partial("_ReservedNamespacesList", Model.ReservedNamespaces) | |||
There was a problem hiding this comment.
nit: No need to wrap this in @{ ... }
| Value = reservedNamespace.Value; | ||
| IsPublic = reservedNamespace.IsSharedNamespace; | ||
| IsPrefix = reservedNamespace.IsPrefix; | ||
| Owners = reservedNamespace.Owners.Select(owner => owner); |
There was a problem hiding this comment.
Why do you need this Select?
There was a problem hiding this comment.
don't need it, reminiscent of some view model changes I was making. removed it.
| } | ||
| } | ||
|
|
||
| <div class="row user-package-list"> |
There was a problem hiding this comment.
Given that this is the second clone of _UserPackagesList.cshtml that we've implemented, can we create some sort of ViewHelpers function that can create a list like this? Every time one page changes, we have to change every other page.
There was a problem hiding this comment.
Perhaps we can, but I wouldn't want to make it for this PR given that its a much bigger change for refactoring and affecting other components for thorough testing.
| { | ||
| <tr class="manage-package-listing"> | ||
| <td class="align-middle hidden-xs"> | ||
| <img class="reserved-indicator-icon img-responsive" |
There was a problem hiding this comment.
nit: Do we have this same chunk of code everywhere we embed the reserved package icon? Is there some way we could put it in a shared place?
There was a problem hiding this comment.
I don't think we should refactor the view elements much, this isn't exactly the same piece of code, the fallback urls are different, its easier to change the view elements that are aggregated together.
| @@ -161,11 +161,13 @@ public virtual ActionResult Packages() | |||
| var outgoing = _packageOwnerRequestService.GetPackageOwnershipRequests(requestingOwner: user); | |||
|
|
|||
| var ownerRequests = new OwnerRequestsViewModel(incoming, outgoing, user, _packageService); | |||
| var reservedPrefixes = new ReservedNamespaceViewModel(user.ReservedNamespaces); | |||
There was a problem hiding this comment.
Are we concerned we're adding too many lists to ManagePackages? It has the user's packages, their owner requests, and now their reserved namespaces. It's ok now, but it just seems like we're dumping tons of content there.
There was a problem hiding this comment.
There isn't a better place for this, I don't think this warrants another page either. Also, given that this would show up only if you have a reserved namespace and same goes with owner requests I think its fine.
The view looks like this: