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

Fix #6622 via CSS (multiline on verify page) #7501

Merged
merged 3 commits into from Sep 20, 2019

Conversation

@robertmuehsig
Copy link
Contributor

commented Sep 3, 2019

As suggested in the issue, this should solve the bug. Not sure if I found the correct location for this less changes.

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

  • Added a new less rule to display multiline in a correct way
  • The CSS is should only affect the verify package site.
  • Didn't found any other CSS/LESS stuff, so I used the "common-edit-metadata.less" file.

Works in more or less all modern browsers :)

Addresses #6622

As suggested in the issue, this should solve the bug. Not sure if I found the correct location for this css.
@joelverhagen

This comment has been minimized.

Copy link
Member

commented Sep 5, 2019

This is a different implementation that the display package page due to complexities mentioned by @scottbommarito in the original issue. Won't pre-wrap show consecutive spaces? Wouldn't pre-line be a bit closer to the display package behavior?

I am kind of curious how tab characters are displayed... in both cases 😄

Let's just make sure we're picking the most appropriate white-space value then we can :shipit:.

@robertmuehsig

This comment has been minimized.

Copy link
Contributor Author

commented Sep 5, 2019

Ok, I tried something. I changed the description from the original package and uploaded it to my dev environment:

Screenshot from the display package page:

image

The content was changed like this:

image

Now let's see how pre-wrap renders the page:

image

Now with pre-line:

image

To compare - this would be rendered without the PR:

image

I think pre-line is the better choice here - at least it is more consistent with the display package page.

Copy link
Member

left a comment

I agree with pre-line for now because it aligns with the Display Package page, but I would argue that the Display Package page should behave like pre-wrap...but that's out of scope of this fix.

Copy link
Member

left a comment

/cc @loic-sharma @scottbommarito - can we shepherd this change into dev branch?

@joelverhagen

This comment has been minimized.

Copy link
Member

commented Sep 11, 2019

Great analysis, @robertmuehsig!

@joelverhagen joelverhagen changed the base branch from master to dev Sep 11, 2019
@joelverhagen

This comment has been minimized.

Copy link
Member

commented Sep 11, 2019

Changed the base branch to dev.

@joelverhagen

This comment has been minimized.

Copy link
Member

commented Sep 20, 2019

Looks like this still has a different behavior for indentation done with space characters. We can't win all the battles I think with just CSS so this is just fine for now.

@joelverhagen joelverhagen merged commit 90eb2bb into NuGet:dev Sep 20, 2019
2 checks passed
2 checks passed
NuGetGallery - CI #62167 succeeded
Details
license/cla All CLA requirements met.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.