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

[TFM Display] Badges and Table UI. #8885

Merged
merged 21 commits into from
Jan 14, 2022
Merged

[TFM Display] Badges and Table UI. #8885

merged 21 commits into from
Jan 14, 2022

Conversation

dannyjdev
Copy link
Contributor

@dannyjdev dannyjdev commented Nov 19, 2021

Description

  • HTML partials added for badges and table components for DisplayPackagePageV2.
  • TFM feature flag and flight added.
  • PackageFrameworkCompatibilityFactory now returns an immutable dictionary with the first four keys ordered to be .NET the based frameworks (if they have compatible frameworks for the package), the remaining keys are ordered by alphabetical order.
  • GetBadgeVersion() extension method returns the version that we are going to display for the .NET badge framework.
  • Checked accessibility (needed to change color due to contrast a11y issue).

Addresses

Screenshots

Package with supported frameworks, table and badges shown in UI

Iteration 3 - Package with table and badges

Package with supported frameworks, but not .NET framework in asset. NOTE: On badges we only show a .NET based framework if it's in the assets.

Iteration 3 - Package with table and no badges, with net computed framework

Package with no supported frameworks in their assets.

NOTE: Badges are not shown here, and since there are no supported assets we provide a little message.
Iteration 3 - No supported frameworks, no table nor badges

Newtonsoft.json before removing deprecated/unofficial frameworks

Iteration 2 - Newtonsoft

Newtonsoft.json after removing deprecated/unofficial frameworks

Iteration 3 - Newtonsoft

Package with only netstandard1.0

Iteration 3 - Package only netstandard1

Package with net5.0-windows10.0.17763

Iteration 3 - net5 specific windows version

Package with only net5/6 windows10.0.18362

Iteration 3 - only net5 and 6 specific windows versions

@dannyjdev dannyjdev marked this pull request as ready for review January 10, 2022 19:45
@dannyjdev dannyjdev requested a review from a team as a code owner January 10, 2022 19:45
@clairernovotny
Copy link
Contributor

Overall it looks good, but I think the display looks very busy, because of all of the computed boxes.

Few thoughts here:

  1. We show old frameworks/tfm's like dnx that aren't really supported. Can we remove those and only display things that were shipped? I would not consider dnx/aspnet as shipped tfm's.
  2. Showing all of the computed tfm's adds a lot of noise. When we add .NET 7/ios/android, etc, it's only going to get worse. Maybe this can be improved by hiding computed tfm's by default and having a toggle to display them? Or a user preference to display them?

@JonDouglas
Copy link
Contributor

This looks...AMAZING! GREAT JOB! Seriously, this looks great!

Checked accessibility (needed to change color due to contrast a11y issue).

P.S. Are we not able to use an accent color like light blue or orange for these that complements say the border? i.e. #0078d4

@JonDouglas
Copy link
Contributor

This NuGet package contains no supported frameworks in their assets.

There are no supported framework assets in this package.

Package asset frameworks

Compatible target framework(s)

Additional computed frameworks

Additional computed target framework(s)

@joelverhagen
Copy link
Member

Aligning with @clairernovotny's first point -- I think we should hide dnx and aspnet unless that package has actually assets category. In other words, if that category only has computed frameworks, hide it.

Could you show Newtonsoft.Json and one other package with only a netstandard package asset?

@JonDouglas
Copy link
Contributor

JonDouglas commented Jan 10, 2022

I wrote about this in a spec somewhere, but we should consider removing from this list the "deprecated frameworks":

https://docs.microsoft.com/en-us/nuget/reference/target-frameworks#deprecated-frameworks

I believe it's fine as it is now and we can iterate on it as we get more feedback.

@joelverhagen
Copy link
Member

@JonDouglas, thanks for the clarity. Sounds good to me. @dannyjdev feel free to proceed without my first comment.

The additional screenshots will help so please add those.

Could you show Newtonsoft.Json and one other package with only a netstandard package asset?

@skofman1
Copy link
Contributor

Perhaps it's already in the UX design (but not part of this PR) - I wonder if we can use the real estate in this tab to help users learn about TFMs and TFM compatibility. This can be especially useful for new .NET users. Are there docs we can link? A 101 video we can point to?

}
else
{
<span tabindex="0">This NuGet package contains no supported frameworks in their assets.</span>
Copy link
Contributor

Choose a reason for hiding this comment

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

Do our users understand what this means and its implications?

Feel free to ignore this feedback if this is not a common category (few packages with few downloads)

Copy link
Contributor Author

@dannyjdev dannyjdev Jan 12, 2022

Choose a reason for hiding this comment

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

I added a "Learn more about..." link for the tab, it will appear if the package contains or not frameworks, so that users can learn about TFM and .NET. Please refer to the screenshot "Package with no supported frameworks in their assets."

<div>
<i class="ms-Icon ms-Icon--SquareShape frameworktableinfo-computed-icon"></i>
<span class="frameworktableinfo-text">Additional computed frameworks</span>
</div>
Copy link
Contributor

@loic-sharma loic-sharma Jan 10, 2022

Choose a reason for hiding this comment

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

Do users understand what a Compatible target framework(s) or Additional computed frameworks mean? Do they understand the implication of a TFM being a computed framework versus an asset framework?

Copy link
Contributor

Choose a reason for hiding this comment

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

We definitely need some doc snippets about framework precedence & direct matches. Maybe we do that here? https://docs.microsoft.com/en-us/nuget/consume-packages/finding-and-choosing-packages once we ship?

@dannyjdev
Copy link
Contributor Author

Aligning with @clairernovotny's first point -- I think we should hide dnx and aspnet unless that package has actually assets category. In other words, if that category only has computed frameworks, hide it.

Could you show Newtonsoft.Json and one other package with only a netstandard package asset?

Please check the 2 latest screenshots I added to the description of this issue.

Copy link
Contributor

@JonDouglas JonDouglas left a comment

Choose a reason for hiding this comment

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

Wonderful job. All my concerns are addressed. There are definitely things we can improve on this in future iterations, but the current experience is how we start to get more feedback from our community. Seriously, great job from spec -> implementation here. Impressive work!

@joelverhagen
Copy link
Member

Wow, there's a LOT of weird frameworks on the netstandard one. I think it's still fine to wait and see with customer feedback on including them (per @JonDouglas's feedback). But it might be overwhelming to new users or shed light on some very esoteric frameworks that we don't want to spend energy on.

@anangaur
Copy link
Member

anangaur commented Jan 11, 2022

Looks awesome 👏 Minor comments:

  • Should the legends for compatible and additional computed frameworks be at the top? (May be already discussed and decided based on UX feedback but wanted to put this here nevertheless)
  • Can there be "Learn more about framework compatibility here" linking to a docs page near the legend or at the top of the tab page? Will help new users unaware with the concept or folks who are startled by the amount of info on this tab.

@JonDouglas
Copy link
Contributor

JonDouglas commented Jan 11, 2022

Two last questions as I took another quick look:

  1. The badge at the top shows the lowest .NET Framework supported. Don't we want to have some type of "+" sign or "or higher" if there is another supported framework version that is higher than it? Or we just show the latest .NET Framework given precedence rules?

Scenario 1. netstandard1.0, netstandard1.3, netstandard2.0 -> .NET Standard 1.0+ / .NET Standard 1.0. (or higher)

Scenario 2. netstandard1.0, netstandard1.3, netstandard2.0 -> .NET Standard 2.0

image

Latest is greatest? Maybe?

  1. TFM compat is ordered by numeric version. Should we group these by compat/computed compat instead so a user can quickly glance on the left & see what has assets vs. what is computed for precedence?

i.e.

image

Thoughts anyone?

@joelverhagen
Copy link
Member

Don't we want to have some type of "+" sign or "or higher" if there is another supported framework version that is higher than it?

Yes, I agree. This emphasizes the support "range" rather than a specific framework. We should do this IMO.

TFM compat is ordered by numeric version.

I have no strong feeling here, but I think the numeric ordering is a bit easier to read rather than grouping by "has package assets" first and then by number ordering.

@dannyjdev
Copy link
Contributor Author

Perhaps it's already in the UX design (but not part of this PR) - I wonder if we can use the real estate in this tab to help users learn about TFMs and TFM compatibility. This can be especially useful for new .NET users. Are there docs we can link? A 101 video we can point to?

This was discussed with PM/UX when the design was created, but ended up not adding. Since this is something that could be easily added I'm discussing it with @JonDouglas offline.

@JonDouglas
Copy link
Contributor

JonDouglas commented Jan 11, 2022

Should the legends for compatible and additional computed frameworks be at the top? (May be already discussed and decided based on UX feedback but wanted to put this here nevertheless)

Since most packages do not target the entire world, I think it's fine as is. But maybe we change later if it makes sense?

Can there be "Learn more about framework compatibility here" linking to a docs page near the legend or at the top of the tab page? Will help new users unaware with the concept or folks who are startled by the amount of info on this tab.

We're working to get this included as a footnote of the legend.

@JonDouglas
Copy link
Contributor

Can we do a cross comparison of current supported frameworks both in code / docs & within our mappings?

https://docs.microsoft.com/en-us/nuget/reference/target-frameworks#supported-frameworks

For example TFM like netstandardapp15 and netstandard1.7 may fall under those deprecated / special cases of packages may support it, but it never made an official release.

From another issue about netstandard1.7:

It's a temporary place holder until netstandard 2.0 is ready in the toolchain (specifically nuget)

Similarly netstandardapp15 which was lingering around the dnx timeframe but isn't seen as an official netstandard TFM.

I'm going to add a couple client folks to help review TFM here for comments. This will help add some polishing touches 🥇

@nkolev92 @zivkan would you be able to give this a quick review based on screenshots?

@zivkan
Copy link
Member

zivkan commented Jan 12, 2022

While I agree with other people's comments that the compatible TFM list looks very busy and I imagine it would be overwhelming to people new to .NET, but I have nothing to add and I'm certainly not a UX expert.

From a technical/NuGet Client point of view, looking only at the screenshots, not the code, it looks fine.

As others mentioned, .NET Standard 1.7 isn't really a thing. It's not listed in the .NET team's own docs on .NET Standard. So, I'd say it should be removed, and maybe only shown when a package contains netstandard1.7 assets.

Having a quick look through other people's comments, I don't see .NET (5+) platform version compatibility being raised. While the .NET SDK allows projects to not provide a platform version, and will therefore use its default, customers can opt into higher platform versions if they want to get access to newer APIs.

For example, 7.0 is the default platform version for Windows, which means that customers do not have access to Windows 10 APIs. Anyone creating a WinUI needs to make their project, and any packages they create, target net5.0-windows10.0.??? where ??? is whatever Windows SDK version they want to target, for example 10.0.18362.0. Any project targeting just net5.0-windows, which is implicitly net5.0-windows7.0 is not compatible with that net-5.0-windows10.0.18362.0 package.

The same goes for android, ios, maccatalyst, and so on. It's just that there are different default versions for each.

One idea is to display the full NuGet Framework (technically TFM standards for Target Framework Moniker, which is the .NETCoreApp,Version=v5.0 part of net5.0-windows, which as you can see is missing platform information), rather than cutting off the platform version. What I mean is display net5.0-windows10.0.18362.0 instead of just net5.0-windows. I am NOT proposing to display all known platform versions. I don't think we should show net5.0-windows7.0 as a compatible TFM in addition to saying that the package contains net5.0-windows10.0.???.

The problem is that most customers won't know what the default platform version is for each .NET SDK, so they won't know that net5.0-windows and net5.0-windows7.0 are equivalent. When the nupkg contains assets for the default platform version it might be nice for Gallery to show net5.0-windows instead of net5.0-windows7.0, however this knowledge is only in the .NET SDK in the MSBuild targets file linked above, there's no API to obtain it.

@dannyjdev
Copy link
Contributor Author

Can we do a cross comparison of current supported frameworks both in code / docs & within our mappings?

https://docs.microsoft.com/en-us/nuget/reference/target-frameworks#supported-frameworks

For example TFM like netstandardapp15 and netstandard1.7 may fall under those deprecated / special cases of packages may support it, but it never made an official release.

From another issue about netstandard1.7:

It's a temporary place holder until netstandard 2.0 is ready in the toolchain (specifically nuget)

Similarly netstandardapp15 which was lingering around the dnx timeframe but isn't seen as an official netstandard TFM.

I'm going to add a couple client folks to help review TFM here for comments. This will help add some polishing touches 🥇

@nkolev92 @zivkan would you be able to give this a quick review based on screenshots?

Some of these frameworks that are computed were retrieved from the FrameworkConstants
class like NetStandard17 and NetStandardApp15, but if they are not official releases I will remove them.

@dannyjdev
Copy link
Contributor Author

@zivkan I just added 2 screenshots on how it will look if a package contains a specific net5.0-windows version, for computing additional frameworks we only have empty versions for those platforms, but if a user has a package with an asset for an specific version, the whole version will show as you can see on the screenshots.

@zivkan
Copy link
Member

zivkan commented Jan 12, 2022

I just added 2 screenshots

From a UX perspective, particularly for newbies, I have no idea, so I'm not even going to try to comment.

From an advanced .NET developer and technical point of view, looks good to me.

That package that has net5.0 as well as net5.0-windows10.0.x packages also listing net5.0-windows as compatible at first feels wrong, but in fact I imagine it's correct and that client would actually select the net5.0 assets, whereas the customer might expect it to select the windows assets. But that's not compatible with the customer's net5.0-windows7.0 package. It goes to show how complex all this package compatibility stuff is.

Anyway, lgtm from my end 👍

@dannyjdev dannyjdev merged commit 8b7df7d into dev Jan 14, 2022
@dannyjdev dannyjdev deleted the dev-dj-tfm-displaypage-ui branch January 14, 2022 19:56
@advay26 advay26 mentioned this pull request Jan 25, 2022
9 tasks
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.

8 participants