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

Add Tech Spec for rendering README in PM UI #13200

Open
wants to merge 19 commits into
base: dev
Choose a base branch
from

Conversation

jgonz120
Copy link

Tech spec for #12583

@jgonz120 jgonz120 requested a review from a team as a code owner January 26, 2024 20:09
Copy link
Contributor

@jebriede jebriede left a comment

Choose a reason for hiding this comment

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

Looks good! Left a few suggestions.

accepted/2024/[TS]PMUI-Readme-rendering.md Show resolved Hide resolved
accepted/2024/[TS]PMUI-Readme-rendering.md Outdated Show resolved Hide resolved
accepted/2024/[TS]PMUI-Readme-rendering.md Outdated Show resolved Hide resolved
accepted/2024/[TS]PMUI-Readme-rendering.md Outdated Show resolved Hide resolved
accepted/2024/[TS]PMUI-Readme-rendering.md Show resolved Hide resolved
accepted/2024/[TS]PMUI-Readme-rendering.md Show resolved Hide resolved
@JonDouglas
Copy link
Contributor

Looks good left a few comments to help

Copy link
Contributor

@donnie-msft donnie-msft left a comment

Choose a reason for hiding this comment

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

Great progress - there's a lot of cool stuff coming with this work!

btw, I'd probably edit the title of the PR to mention README.

accepted/2024/[TS]PMUI-Readme-rendering.md Outdated Show resolved Hide resolved
accepted/2024/[TS]PMUI-Readme-rendering.md Outdated Show resolved Hide resolved
accepted/2024/[TS]PMUI-Readme-rendering.md Outdated Show resolved Hide resolved
accepted/2024/[TS]PMUI-Readme-rendering.md Show resolved Hide resolved
accepted/2024/[TS]PMUI-Readme-rendering.md Outdated Show resolved Hide resolved
accepted/2024/[TS]PMUI-Readme-rendering.md Outdated Show resolved Hide resolved
accepted/2024/[TS]PMUI-Readme-rendering.md Outdated Show resolved Hide resolved
accepted/2024/[TS]PMUI-Readme-rendering.md Outdated Show resolved Hide resolved
accepted/2024/[TS]PMUI-Readme-rendering.md Outdated Show resolved Hide resolved
accepted/2024/[TS]PMUI-Readme-rendering.md Outdated Show resolved Hide resolved
accepted/2024/[TS]PMUI-Readme-rendering.md Outdated Show resolved Hide resolved
@nkolev92
Copy link
Member

Review Meeting:
@jgonz120
Has all the feedback here been addressed?
Ready for another review?

Copy link
Member

@nkolev92 nkolev92 left a comment

Choose a reason for hiding this comment

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

LGTM 👏

A couple of experience questions about the details and readme tabs.

accepted/2024/[TS]PMUI-Readme-rendering.md Show resolved Hide resolved
accepted/2024/[TS]PMUI-Readme-rendering.md Show resolved Hide resolved
@nkolev92
Copy link
Member

nkolev92 commented Jul 9, 2024

Can we update the title of this PR to something more representative?

@jgonz120 jgonz120 changed the title Add file for PM UI design Add Tech Spec for rendering README in PM UI Jul 9, 2024
@zivkan zivkan marked this pull request as draft July 23, 2024 22:34
@jgonz120 jgonz120 marked this pull request as ready for review August 8, 2024 00:19
...
}
```
##### PackageBaseAddress/6.12.0

A new version of the [package content](https://learn.microsoft.com/en-us/nuget/api/package-base-address-resource) resource type which will include a url definition for downloading the README.
Copy link
Member

Choose a reason for hiding this comment

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

Pedantic: This example below is not showing the URL to download the readme, it's showing a base URL, for which a URL has to be generated. It also doesn't tell us what the generated URL will be. See the docs for examples how they explain to both service and client implementors what the expected URL will be.

Guessing from the registration example's rawReadMeUrl, I assume the proposed filename will be readme. Consider the two documented resources both have file extensions, and embedded readmes only support markdown, should it be readme.md?

Copy link
Author

Choose a reason for hiding this comment

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

I can update the doc, as for the URI. Users can pick whatever name they want for the README. If we set the api {@id}/{LOWER_ID}/{LOWER_VERSION}/readme.md it implies like are specifically looking for that file. As opposed to /readme which leaves it open to whatever readme is available for that package.

Copy link
Member

Choose a reason for hiding this comment

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

yes, the doc needs to explain how server implementors tell client what filename to use (what pattern client will use to generate the URl that will be requested)

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.

9 participants