Polish upload UI for license html file #8735

lyndaidaii · 2021-08-04T18:36:49Z

Summary of the changes (in less than 80 characters):
Upload page:

Display page:

Addresses #8277

joelverhagen · 2021-08-04T18:41:05Z

There is at least one warning that can be produced by the Markdown: disallowed image host. Are these warnings surfaced similarly for license Markdown? I think there may be other warnings too... perhaps HTTP images or other things? Let's make sure there is parity in these edge cases.

joelverhagen · 2021-08-04T21:22:44Z

src/Bootstrap/less/theme/common-licenses.less

@@ -4,7 +4,14 @@
        word-break: normal;
    }

-    .custom-license-container {
+    .custom-license-container, .license-file-html-container{


have you verified plaintext licenses? This looks like this might be a change in UI for the custom-license-container class (which I am assuming is related to the existing plain text embedded license file).

yep, it will be plaintext with block display to match with html display.

src/NuGetGallery/Views/Packages/License.cshtml

zhhyu · 2021-08-07T01:11:53Z

                <label>License text</label>

Should we show "License text" for licenses in Markdown?

Refers to: src/NuGetGallery/Views/Packages/License.cshtml:67 in 1deecfd. [](commit_id = 1deecfd, deletion_comment = False)

zhhyu · 2021-08-07T01:21:37Z

src/NuGetGallery/Views/Packages/License.cshtml

-                            @Html.Raw(Model.LicenseFileContentsHtml)
-                        } 
-                        else 
+                            if (Model.LicenseFileContentsHtml.ImageSourceDisallowed)


For readmes, I think we only show the warning message to the package owner. For licenses, will it show this to everyone?

I added logic to show warning to author only

zhhyu · 2021-08-07T01:28:28Z

src/NuGetGallery/Views/Packages/License.cshtml

-                            @Html.Raw(Model.LicenseFileContentsHtml)
-                        } 
-                        else 
+                            if (Model.LicenseFileContentsHtml.ImageSourceDisallowed)


Are there any other cases of warning messages? such as "ImagesRewritten"?

Included "ImagesRewritten" warning now

zhhyu · 2021-08-07T01:31:11Z

src/NuGetGallery/Views/Packages/_VerifyMetadata.cshtml

@@ -135,7 +135,10 @@
            <div class="verify-package-field common-licenses">
                <label class="verify-package-field-heading">License file</label>
                <!-- ko if: $data.LicenseFileContentsHtml -->
-                <span data-bind="html: $data.LicenseFileContentsHtml"></span>
+                <!-- ko if: $data.LicenseFileContentsHtml.ImageSourceDisallowed-->
+                @ViewHelpers.AlertWarning(@<text>Some images are not displayed as they are not from <a href='https://aka.ms/nuget-org-readme#allowed-domains-for-images-and-badges'>trusted domains</a>.</text>)


If we reuse the message for multiple times, it will be better that we put it in a shared place: src/NuGetGallery/Strings.resx

Created function in viewHelpers since we use this alertWarning multiple times

zhhyu · 2021-08-07T01:39:04Z

src/NuGetGallery/Helpers/ViewModelExtensions/DisplayLicenseViewModelFactory.cs

@@ -68,7 +68,7 @@ public DisplayLicenseViewModelFactory(IIconUrlProvider iconUrlProvider, IMarkdow
                package.EmbeddedLicenseType == EmbeddedLicenseFileType.Markdown && 
                licenseFileContents != null)
            {
-                viewModel.LicenseFileContentsHtml = _markdownService.GetHtmlFromMarkdown(licenseFileContents)?.Content;
+                viewModel.LicenseFileContentsHtml = _markdownService.GetHtmlFromMarkdown(licenseFileContents);


Will it be possible to add some tests here?

Test was added

zhhyu · 2021-08-07T01:53:55Z

src/NuGetGallery/Views/Packages/_VerifyMetadata.cshtml

+                <!-- ko if: $data.LicenseFileContentsHtml.ImageSourceDisallowed-->
+                @ViewHelpers.AlertWarning(@<text>Some images are not displayed as they are not from <a href='https://aka.ms/nuget-org-readme#allowed-domains-for-images-and-badges'>trusted domains</a>.</text>)
+                <!-- /ko -->
+                <span class="license-file-html-container" data-bind="html: $data.LicenseFileContentsHtml.Content"></span>


I am a little curious about why using span here?

It was span before, I just follow the original code adding class to it <span data-bind="html: $data.LicenseFileContentsHtml"></span>

zhhyu · 2021-08-07T02:22:15Z

src/Bootstrap/less/theme/common-licenses.less

+        display: block;
+        padding: 10.5px;
+        background-color: @pre-bg;
+        border: 1px solid #ccc;


My apologies that I didn't check the css structure at Verify page. I see some properties such as "border". Should they share with other containers like embedded readmes or inherit from the parent?

license container is sharing the embedded readme css setting on verify page. One thing we could do is to make common-licenses.less and common-readme.less to one file, it will make less duplicate. Include in the base.less probably those two are not general enough. Please let me know your thoughts. thanks

zhhyu · 2021-08-07T02:28:02Z

src/NuGetGallery/Views/Packages/License.cshtml

+                                @ViewHelpers.AlertWarning(@<text>Some images are not displayed as they are not from <a href='https://aka.ms/nuget-org-readme#allowed-domains-for-images-and-badges'>trusted domains</a>.</text>)
+
+                            }
+                            @Html.Raw(Model.LicenseFileContentsHtml.Content)


I suggest to check the rendering/css with Verify page to ensure that they will share the same appearance as much as possible. This is critical to understand what it will look like before the package is pushed.

Besides, I can see that the text license file has the background color, will the markdown one share the similar appearance?
https://www.nuget.org/packages/license-file-lucene-1/0.0.2/License

No background color, here is what it will looks like

joelverhagen

Address comments then

lyndaidaii · 2021-08-25T21:57:40Z

                <label>License text</label>
Should we show "License text" for licenses in Markdown?

Refers to: src/NuGetGallery/Views/Packages/License.cshtml:67 in 1deecfd. [](commit_id = 1deecfd, deletion_comment = False)

I changed to License file instead of License text

…nse-lynn

agr · 2021-08-31T21:37:49Z

src/NuGetGallery/App_Code/ViewHelpers.cshtml

+{
+    @AlertWarning(
+    @<text>
+        This documentation had some images automatically rewritten to use secure links and may be broken.


"This documentation had some image URLs automatically rewritten ..." maybe? We don't change images themselves, do we?

we don't change the images, but we changes the images source to https if it comes from http

lyndaidaii requested a review from a team as a code owner August 4, 2021 18:36

polish upload for license html file

681c64b

lyndaidaii force-pushed the license-lynn branch from 20ffb00 to 681c64b Compare August 4, 2021 18:39

display

1deecfd

joelverhagen reviewed Aug 4, 2021

View reviewed changes

zhhyu reviewed Aug 7, 2021

View reviewed changes

src/NuGetGallery/Views/Packages/License.cshtml Show resolved Hide resolved

zhhyu reviewed Aug 7, 2021

View reviewed changes

joelverhagen approved these changes Aug 13, 2021

View reviewed changes

add unit test & add imageRewritted alert

8d2dbf3

joelverhagen approved these changes Aug 26, 2021

View reviewed changes

Merge branch 'dev' of https://github.com/NuGet/NuGetGallery into lice…

b2ef467

…nse-lynn

agr reviewed Aug 31, 2021

View reviewed changes

agr approved these changes Aug 31, 2021

View reviewed changes

loic-sharma approved these changes Aug 31, 2021

View reviewed changes

lyndaidaii merged commit 9dd5b06 into dev Aug 31, 2021

dannyjdev mentioned this pull request Sep 1, 2021

[Deployment] 2021.08.30 #8775

Closed

6 tasks

joelverhagen deleted the license-lynn branch August 22, 2024 16:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Polish upload UI for license html file #8735

Polish upload UI for license html file #8735

lyndaidaii commented Aug 4, 2021 •

edited

Loading

joelverhagen commented Aug 4, 2021

joelverhagen Aug 4, 2021

lyndaidaii Aug 4, 2021

zhhyu commented Aug 7, 2021

zhhyu Aug 7, 2021

lyndaidaii Aug 25, 2021

zhhyu Aug 7, 2021

lyndaidaii Aug 25, 2021

zhhyu Aug 7, 2021 •

edited

Loading

lyndaidaii Aug 25, 2021

zhhyu Aug 7, 2021 •

edited

Loading

lyndaidaii Aug 25, 2021

zhhyu Aug 7, 2021 •

edited

Loading

lyndaidaii Aug 25, 2021

zhhyu Aug 7, 2021

lyndaidaii Aug 25, 2021

zhhyu Aug 7, 2021 •

edited

Loading

lyndaidaii Aug 25, 2021

joelverhagen left a comment

lyndaidaii commented Aug 25, 2021

agr Aug 31, 2021 •

edited

Loading

lyndaidaii Aug 31, 2021

Polish upload UI for license html file #8735

Polish upload UI for license html file #8735

Conversation

lyndaidaii commented Aug 4, 2021 • edited Loading

joelverhagen commented Aug 4, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhhyu commented Aug 7, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhhyu Aug 7, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhhyu Aug 7, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhhyu Aug 7, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhhyu Aug 7, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joelverhagen left a comment

Choose a reason for hiding this comment

lyndaidaii commented Aug 25, 2021

agr Aug 31, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lyndaidaii commented Aug 4, 2021 •

edited

Loading

zhhyu Aug 7, 2021 •

edited

Loading

zhhyu Aug 7, 2021 •

edited

Loading

zhhyu Aug 7, 2021 •

edited

Loading

zhhyu Aug 7, 2021 •

edited

Loading

agr Aug 31, 2021 •

edited

Loading