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

[Readme]Reduce space when display content in readme container #8609

Merged
merged 13 commits into from
Jun 24, 2021
Merged

Conversation

lyndaidaii
Copy link
Contributor

@lyndaidaii lyndaidaii commented May 26, 2021

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

Reduce space when display readme

Before:
uploadbefore
manageReadme

After:
uploadPageAfter
editRee

Addresses #8508, #8651

@lyndaidaii lyndaidaii requested a review from a team as a code owner May 26, 2021 21:34
@lyndaidaii lyndaidaii changed the title Reduce space when display content in readme container [Readme]Reduce space when display content in readme container May 26, 2021
@@ -87,7 +87,7 @@
</div>
</div>

<div class="hidden" id="readme-preview" style="padding-top:3em;">
<div class="hidden" id="readme-preview" style="padding-top:0.25em;">
Copy link
Member

Choose a reason for hiding this comment

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

Could we move this inline CSS to a LESS file that is appropriate for all of the places _ImportReMe.cshtml is used?

@joelverhagen
Copy link
Member

It seems that there is also a lot of extra space below the content. Is that intentional?
image

@lyndaidaii
Copy link
Contributor Author

It seems that there is also a lot of extra space below the content. Is that intentional?
image

This is intential, the readme container size has default size.

@loic-sharma
Copy link
Contributor

This is intential, the readme container size has default size.

What's the purpose of the container's default size? The forced size looks a little strange to me.

@lyndaidaii
Copy link
Contributor Author

This is intential, the readme container size has default size.

What's the purpose of the container's default size? The forced size looks a little strange to me.

@chgill-MSFT, could you help provide some idea in terms of extra space below content?

@lyndaidaii
Copy link
Contributor Author

lyndaidaii commented Jun 22, 2021

image

The space here actually is div for edit markdown
<div class="text-center hidden" id="edit-markdown" style="padding-top:3em; padding-bottom:3em"> <div class="row"> <input type="button" value="Edit Documentation" id="edit-markdown-button" class="btn btn-primary col-xs-10 col-xs-offset-1 col-lg-2 col-lg-offset-5" /> </div> </div>
https://github.com/NuGet/NuGetGallery/blob/main/src/NuGetGallery/Views/Packages/_ImportReadMe.cshtml#L96
When we preview readme content, we remove hidden class
' function displayReadMePreview(response) {
$("#readme-preview-contents").html(response.Content);
$("#readme-preview").removeClass("hidden");

        $('.readme-tabs').children().hide();

        $("#edit-markdown").removeClass("hidden");
        $("#preview-html").addClass("hidden");
        clearReadMeError();'

https://github.com/NuGet/NuGetGallery/blob/main/src/NuGetGallery/Scripts/gallery/page-edit-readme.js#L332

I created new ticket to address seperately, It was not trivial fix, here is ticket #8651

@lyndaidaii
Copy link
Contributor Author

lyndaidaii commented Jun 23, 2021

After syncing with Joel, we found alternative approach to clean up space issue below content. This PR is addressing both #8508, #8651. Tested cases like uploading package with legacy readme and embedded readme, edit legacy/embedded readme for different version of packages


#preview-html {
padding-top: 3em;
padding-bottom: 3em
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
padding-bottom: 3em
padding-bottom: 3em;

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