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

New Display Package Page: created flight and setup for implementation of new page #8591

Merged
merged 6 commits into from
May 26, 2021

Conversation

sophiamfavro
Copy link
Contributor

Created a flight for the new page design and created the necessary duplicate files for my changes. I also made all the necessary additions so those files would work if flight turned off.

If you have any questions for why I am doing this look at the Display Package Page Redesign Engineering spec, comment, or reach out to me.

@sophiamfavro sophiamfavro requested a review from a team as a code owner May 22, 2021 00:55
@@ -0,0 +1,327 @@
.page-package-details-v2 {
Copy link
Contributor

@loic-sharma loic-sharma May 24, 2021

Choose a reason for hiding this comment

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

For folks reviewing, this file is intended to be almost identical to the original page-display-package.less file. This first line is the only difference:

- .page-package-details {
+ .page-package-details-v2 {
  ...
}

@@ -0,0 +1,173 @@
$(function () {
Copy link
Contributor

@loic-sharma loic-sharma May 24, 2021

Choose a reason for hiding this comment

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

For folks reviewing, this file is intended to be completely identical to page-display-package.js

@@ -321,7 +321,7 @@
-->
<system.web>
<httpCookies requireSSL="true" httpOnlyCookies="true"/>
<compilation debug="false" targetFramework="4.7.2">
<compilation debug="true" targetFramework="4.7.2">
Copy link
Contributor

Choose a reason for hiding this comment

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

Please undo the changes in this file. We have a security tool that complains if these values change. See this conversation for more information.

@helper MakeLicenseLink(CompositeLicenseExpressionSegment segment) {<a href="@LicenseExpressionRedirectUrlHelper.GetLicenseExpressionRedirectUrl(segment.Value)" aria-label="License @segment.Value">@segment.Value</a>}
@helper MakeLicenseSpan(CompositeLicenseExpressionSegment segment) {<span>@segment.Value</span>}

<section role="main" class="container main-container page-package-details-v2">
Copy link
Contributor

Choose a reason for hiding this comment

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

For folks reviewing, this file is intended to be almost identical to the original DisplayPackage.cshtml file. This line is the only difference:

...
-<section role="main" class="container main-container page-package-details">
+<section role="main" class="container main-container page-package-details-v2">
...
</section>
    ...

Comment on lines 293 to 333
if (Model.ValidatingTooLong)
{
@ViewHelpers.AlertWarning(
@<text>
<strong>Package validation is taking longer than expected.</strong> This is not normal, but we are on it!
</text>
)
}
}

@if (Model.FailedValidation)
{
@ViewHelpers.AlertDanger(
@<text>
<strong>Package publishing failed.</strong> This package could not be published due to the following reason(s):
<ul class="failed-validation-alert-list">
@foreach (var issue in Model.PackageValidationIssues)
{
<li>@Html.Partial("_ValidationIssue", issue)</li>
}
</ul>
@if (!Model.PackageValidationIssues.Any(i => i.IssueCode == ValidationIssueCode.Unknown))
{
var issuePluralString = Model.PackageValidationIssues.Count > 1 ? "all the issues" : "the issue";
<text>Once you've fixed @issuePluralString with your package, you can reupload it with the same ID and version.</text>
}
else
{
<text>Please contact <a href="mailto:[email protected]">[email protected]</a> to help fix your package.</text>
}
</text>)
}

@if (Model.Deleted)
{
@ViewHelpers.AlertDanger(
@<text>
<strong>This package has been deleted from the gallery.</strong> It is no longer available
for install/restore.
</text>
)
Copy link
Contributor

@loic-sharma loic-sharma May 24, 2021

Choose a reason for hiding this comment

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

Hey it looks like the spacing here got messed up by the code formatter. See: https://www.diffchecker.com/cry5kiSx

Please make sure there's no weird styling here.

@loic-sharma
Copy link
Contributor

⚠TODO: Let's add a test on which view was returned depending on the feature flag:

  • If the feature flag is off, DisplayPackage view should be returned
  • If the feature flag is on, DisplayPackageV2 view should be return.

@@ -2317,6 +2318,9 @@
<ItemGroup>
<Content Include="Views\Packages\_DisplayPackageVulnerabilities.cshtml" />
</ItemGroup>
<ItemGroup>
<Content Include="Views\Packages\DisplayPackageV2.cshtml" />
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't forget to move this content include into the ItemGroup for the .cshtml files up here:

<Content Include="Views\Packages\DisplayPackage.cshtml" />

@sophiamfavro sophiamfavro merged commit a9d5195 into dev May 26, 2021
@joelverhagen joelverhagen deleted the sofavroPackagePageV2Flight branch August 22, 2024 16:28
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.

3 participants