Search-by-TFM Frontend changes #9342
Conversation
…v' dropdown arrows
|
If people would like me to do a live/sync review meeting for these changes where I walk everyone through the different parts of the changes and show how users can interact with the frontend, leave a like on this message, and I'll look to schedule something in the next few days. |
|
your description of issue is clear, I think it will be better to include the screenshot about search page without your changes in description. It's easier for reviewers to review and compare changes before and after. |
|
I'm concerned that the UI doesn't clarify whether the target framework filters are "exact" (as in the results are packages that have the selected TFMs) or "loose" (as in the results are packages compatible with the selected TFMs). If I'm a newbie working on a .NET 5 app, I might think I need to check .NET 5, .NET 7, and .NET Standard to see all the relevant packages. Some thoughts:
This looks amazing though!!! |
| public string PackageType { get; set; } | ||
|
|
||
| public string SortBy { get; set; } | ||
|
|
||
| public bool ShouldDisplayAdvancedSearchPanel { get; set; } | ||
|
|
||
| public bool IsAdvancedSearchFlightEnabled { get; set; } | ||
|
|
||
| public Dictionary<string, FrameworkFilterGroup> FrameworkFilters { get; set; } |
There was a problem hiding this comment.
Just thought, it might be better and clear to put those frameworkFilter into a helper class instead of PackageListViewModel class.
There was a problem hiding this comment.
I've now moved the Framework filters and the TFM lists to a helper class here: https://github.com/NuGet/NuGetGallery/pull/9342/files#diff-50cad345dfd8a1b562bb127499d6fcff351d75e1f781adc7d4d83a673252b65c
(src/NuGetGallery.Core/Frameworks/FrameworkFilterHelper.cs)
I'm also now using Framework constants from our SupportedFrameworks class instead of direct strings for the TFM filters.
|
What's our rollout plan for the new UX? It would be good to let a small group of users to try out the UX and get early feedback before we roll it out generally. |
|
To @loic-sharma 's comment,
If these are compat (or loose as stated by Loic), would renaming the filter section to /cc: @JonDouglas |
Great points. We will likely work on computed compatibility later. I think we brainstormed a "toggle" type of control to indicate exact/compat, but will probably be a next iteration type of thing while people learn and give us feedback. Checkboxes tend to be what's "in" today for filter experiences in many different ecosystems and marketplaces. Valid points on the radioboxes and we will have to learn first before changing. A tooltip is definitely a nice to have. If we find that users don't find it intuitive enough, we can definitely add one later to explain any "gotchas". Should be fairly straight-forward for now. A future item we wanted to add is a filter badge when it is applied. That way it would show above the search results. Another added bonus type thing. TL;DR - 🚢 🐿️ and let's iterate as we continue to get great feedback. |
|
Small nit: Should framework versions be descending given latest release? |
|
I just realized that the default state of the filter has no indication that it allows all TFMs: |
|
Starting with something like this that would expand into the thing above when appropriate checkboxes are set might make things clearer: |
| const tfms = document.querySelectorAll('[parent=' + this.id); | ||
| tfms.forEach((tfm) => { | ||
| tfm.checked = false; | ||
| updateFrameworkFilters("tfms", tfm.id, false); | ||
| }); |
There was a problem hiding this comment.
Why do we start with none of the TFMs selected? Don't we want all of them if there are none specified by the user?
There was a problem hiding this comment.
Since we combine these filters by 'ANDing' them, the idea here is that with every additional checkbox we check, it restricts/shrinks the result set further, so no checkboxes should mean every package is visible. Other framework/platform filtering experiences seem to work similarly too. I'm open to suggestions on how to make this clearer to users, but checking all checkboxes would imply that a package needs to be targeting 'ALL' of those frameworks.
Also, the framework checkboxes we show here don't cover all frameworks available in our ecosystem (we just show the ones we officially support), so even if we were 'ORing' these filters, it wouldn't technically cover all packages.
There was a problem hiding this comment.
@agr This is in response to your comment here too #9342 (comment)
The default state without checkboxes selected is supposed to indicate that we're not restricting the result set at all, and every additional checkbox we check would make our search more selective. Maybe we can add something more to the tooltip to make this clearer to users?
There was a problem hiding this comment.
@advay26 The part about leaving the checkboxes unchecked to start off with makes sense to me, but the part I'm a bit confused about is the ANDing. Why wouldn't we want to show packages that are applicable for at least 1 of the set of checkboxes selected? For example, when I am filtering products on an online store and I select 3 categories (ex: pants, shirts, hats) I want to see all pants AND all shirts AND all hats, not just products that fall into all 3 categories. I would think that if I check net471, net472, and netcoreapp1.0, I want to see packages that are compatible with any combination of 1, 2 or all 3 of those frameworks, or am I misunderstanding this feature?
There was a problem hiding this comment.
@camigthompson Since package consumers often target multiple frameworks in their projects, they would usually want to search for packages that are compatible with ALL of these target frameworks.
From https://learn.microsoft.com/en-us/nuget/consume-packages/finding-and-choosing-packages#determine-supported-frameworks)
We do want to eventually allow greater flexibility to users who want to search for packages compatible with one framework OR another, and we will look to add that in later iterations of this (alongside computed framework compatibility vs. exact asset frameworks, etc.). Right now, users can still somewhat approximate this by clicking the framework generation checkbox, which in essence searches for packages that have ANY one of the inidividual TFMs within that generation.
| "netstandard2.1" } | ||
| )); | ||
|
|
||
| FrameworkFilters.Add(AssetFrameworkHelper.FrameworkGenerationIdentifiers.NetFramework, |
There was a problem hiding this comment.
Should this hardcoded data model of frameworks be refactored into https://github.com/NuGet/NuGetGallery/blob/4c0074bb3d8a09128994cca3a73661cf502685c4/src/NuGetGallery.Core/Frameworks/SupportedFrameworks.cs?
It seems like we need to reconcile SupportedFrameworks with this somehow since they are connected E2E (e.g. the values selectable in the UI must line up with the values populated into the index by the search jobs).
There was a problem hiding this comment.
I'm now using Framework constants instead of hardcoded strings, and I've placed the lists of TFMs in the SupportedFrameworks class.
| <section role="main" class="container main-container page-list-packages"> | ||
| <div class="row clearfix no-margin"> | ||
| <div class="@(Model.IsAdvancedSearchFlightEnabled ? "col-md-3" : "") no-padding" id="filters-column"> | ||
| @if (Model.IsAdvancedSearchFlightEnabled) |
There was a problem hiding this comment.
I think we need a new flight for this. The existing flight is on in PROD so if we deploy like this the feature will be visible until we turn off the flight. If we rollback, we'll need to remember to turn the flight on. etc etc. Easier to have a separate flight name.
|
Thanks for the feedback everyone! I've addressed most of the comments here and added a few other things:
|
advay26
requested review from
joelverhagen,
agr,
lyndaidaii and
keylime-unicorn
Addresses https://github.com/NuGet/Engineering/issues/4579
This work covers all the frontend changes required to enable Search-by-TFM on NuGet.org:
Previously,
After the changes,
UPDATE: