-
Notifications
You must be signed in to change notification settings - Fork 641
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
[Redesign] Fix README alerts if package hasn't passed validation #8718
Conversation
@@ -539,7 +547,7 @@ | |||
@Html.Raw(Model.ReadMeHtml) | |||
</div> | |||
} | |||
else if (!Model.Deleted && !String.IsNullOrWhiteSpace(Model.Description)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we keep this check if the description doesn't exist?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This check isn't necessary as either ways we'll show nothing if a package has no description. It's basically the same thing as uploading a package with an empty README or description.
FYI, the package's description is required on new packages and there are only 13 packages on nuget.org without a description:
- entropyextension.core/1.0.0-beta-3
- entropyextension.core/1.0.0-beta-4
- entropyextension.core/1.0.0-beta-5
- entropyextension.sqlserver/1.0.0-beta-3
- entropyextension.sqlserver/1.0.0-beta-4
- entropyextension.sqlserver/1.0.0-beta-5
- f-droidapi/0.0.1
- harry.common/0.1.0-alpha1
- harry.common/0.1.0-alpha2
- ngonzalez.util/1.0.0
- saithe/0.1.0
- saithe/0.2.0
- saithe/1.0.0
In other words, this scenario is extremely uncommon
{ | ||
@ViewHelpers.AlertWarning( | ||
@<text> | ||
The readme will become available once package validation has completed successfully. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will it be better if we move this message to the place with other banners such as license, icon, etc.?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question. I'll defer to @jcjiang but will put my thoughts below.
When you upload a package with an embedded README, the README isn't available until the validation completes successfully. This is a confusing experience for customers as they expect the README to be available immediately. It's important we clarify this state with a warning.
Note that you get a sea of warnings when you upload a new package that follows best practices (embedded icon, embedded license file, symbol package, README). If we move the README warning to the header, it'll be yet another amongst many. So if a customer is concerned that their README appears to be missing, they'd have to dig through all these alerts to understand what happened to their README.
On the other hand, adding the warning directly in the README tab content makes it easier to understand why the README content is missing. I can easily spot the relevant warning message detailing it will be available soon.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jcjiang Approved in Teams conversation.
Improve README alerts if package hasn't passed validation
7e850ff
to
69e65a0
Compare
When package with embedded README is uploaded:
If package fails validation and has an embedded README:
Addresses #8716
Test build: https://devdiv.visualstudio.com/DevDiv/_build/results?buildId=5036467&view=results
Test release: https://devdiv.visualstudio.com/DevDiv/_releaseProgress?_a=release-pipeline-progress&releaseId=1113856