Skip to content
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

Organization bug bash: Manage Packages page #5335

Merged
merged 4 commits into from
Jan 24, 2018
Merged

Conversation

chenriksson
Copy link
Member

@chenriksson chenriksson commented Jan 23, 2018

Fixes the following bug bash issues:

Page Screenshot:

image

Package row w/ owners screenshot:

image

}
@if (Model.OwnerRequests.Incoming.RequestItems.Any())
{
@ViewHelpers.Section(this, "requests-incoming",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could probably pretty easily make a helper function to reuse for both Incoming and Outgoing.

@helper OwnerRequestsSection(OwnerRequestListModel model, string name)
{
    @ViewHelpers.Section(this, "requests-" + name",
        @<text>Ownership Requests (@name)</text>,
        @<text>@Html.Raw(model.RequestItems.Count() + " request" + (ownerRequestsIncomingCount == 1 ? "" : "s"))</text>,
        ...
}

You could potentially add this into _OwnerRequestsList itself as well.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will consider in next PR. For #5310 I need to update the namespaces and requests sections to also include organizations and support the account filter at the top.

@@ -4,92 +4,80 @@
ViewBag.Title = "Manage My Package";
ViewBag.Tab = "Packages";
Layout = "~/Views/Shared/Gallery/Layout.cshtml";

var namespacesCount = Model.ReservedNamespaces.ReservedNamespaces.Count();
var ownerRequestsIncomingCount = Model.OwnerRequests.Incoming.RequestItems.Count();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really like the change from "Incoming" to "Received" and from "Outgoing" to "Sent", but ideally we should update the field names on the class to match this naming as well.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

</span>
</td>
<td class="align-middle text-nowrap">
<span data-bind="text: DownloadCount"></span>
<span data-bind="text: ko.unwrap(DownloadCount).toLocaleString()"></span>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like this should be a FormattedDownloadCount computed

@joelverhagen
Copy link
Member

The slash between the package count and the download count seems weird. A comma seems like a better fit.

<a data-bind="attr: { href: ProfileUrl }">
<span data-bind="text: Username"></span>
</a>
<i class="ms-Icon ms-Icon--Group" aria-hidden="true" data-bind="visible: IsOrganization" style="vertical-align: -2px"></i>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Try to avoid inline css. Other icons are doing this.

Copy link
Member

@joelverhagen joelverhagen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Address or ignore comments then :shipit:

@chenriksson chenriksson merged commit d682501 into dev Jan 24, 2018
@chenriksson chenriksson deleted the chenriks-org-bugbash2 branch January 24, 2018 23:52
@chenriksson
Copy link
Member Author

The slash between the package count and the download count seems weird. A comma seems like a better fit.

@joelverhagen Missed this, but will incorporate in a follow-up PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants