-
Notifications
You must be signed in to change notification settings - Fork 642
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
Manage edit documentation #8169
Conversation
src/NuGetGallery/Helpers/ViewModelExtensions/ManagePackageViewModelFactory.cs
Outdated
Show resolved
Hide resolved
@@ -1,4 +1,5 @@ | |||
<form aria-label="Edit your package" id="edit-readme-form"> | |||
|
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.
remove extra line here
Please ignore previous comments. This is new different UI behavior comparing previous one |
return; | ||
} | ||
|
||
if (model.Versions[selectedVersion].HasEmbeddedReadme) { |
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.
It hide edit/submit button when there is embeddedReadme and embeddedReadme feature flag is on. I am considering to remove the feature flag, looks like it's not necessary when roll back case.
For case like we turn off embedded feature, but there are legacy and embedded readme, we still don't allow user to edit embedded readme.
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.
I agree. There is no need to use the feature flag here. The feature flag is used to block customers to upload a package with an embedded readme file. Here checking the DB flag is enough.
/// Signifies whether or not the embedded ReadMe exists | ||
/// </summary> | ||
[NotMapped] | ||
public bool HasEmbeddedReadMe |
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.
I know our codebase is inconsistent between ReadMe
and Readme
, but from my understanding we would like to migrate to the Readme
casing. If so, should we use the Readme
casing on all new properties/methods you introduce?
public bool HasEmbeddedReadMe | |
public bool HasEmbeddedReadme |
var result = new ManagePackageViewModel.VersionReadMeState( | ||
submitUrlTemplate.Resolve(model), | ||
getReadMeUrlTemplate.Resolve(model), | ||
null); |
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.
null [](start = 16, length = 4)
use named params for readability
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.
@@ -156,6 +156,18 @@ public bool HasReadMe | |||
} | |||
} | |||
|
|||
/// <summary> | |||
/// Signifies whether or not the embedded ReadMe exists |
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.
/// Signifies whether or not the embedded ReadMe exists | |
/// Signifies whether or not the embedded Readme exists |
@@ -184,6 +185,22 @@ | |||
if (model === null || !model.IsSymbolsPackage) { | |||
BindReadMeDataManager.bindReadMeData(model); | |||
} | |||
|
|||
|
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.
remove extra line here
@@ -38,6 +38,7 @@ public class DisplayPackageViewModel : ListPackageItemViewModel | |||
public bool IsPackageDependentsEnabled { get; set; } | |||
public NuGetPackageGitHubInformation GitHubDependenciesInformation { get; set; } | |||
public bool HasEmbeddedIcon { get; set; } | |||
public bool HasEmbeddedReadmeFile { get; set; } |
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.
Do we use this property anywhere?
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 catch, it needs to be deleted
@@ -62,7 +62,7 @@ | |||
</div> | |||
</div> | |||
|
|||
@Html.Partial("_EditForm") | |||
@Html.Partial("_EditForm", Model) | |||
</div> |
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.
Do we need to pass the model here? I see that we have passed the model:
"Versions": @Html.ToJson(Model.VersionReadMeStateDictionary), |
@@ -50,6 +50,7 @@ public VersionReadMeState(string submitUrl, string getReadMeUrl, string readMe) | |||
public string SubmitUrl { get; } | |||
public string GetReadMeUrl { get; } | |||
public string ReadMe { get; } | |||
public bool HasEmbeddedReadme { get; set; } |
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.
Do you think whether it's better if we also set this value in the constructor, and restrict it to a read-only field? I see that we only use this field in the corresponding JS file, so it should be safe to do so?
Let's verify the behavior manually, as we don't have a good test coverage on views. |
Summary of the changes:
Documentation UI change in manage package page:
Question for discussion:
Feature flag looks like could be removed here.
For case like we turn off embedded feature, there are legacy and embedded readme, we still don't allow user to edit embedded readme.
Addresses https://github.com/NuGet/Engineering/issues/3307