-
Notifications
You must be signed in to change notification settings - Fork 65
feat: Filter recipes #2354
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
feat: Filter recipes #2354
Conversation
Signed-off-by: Philippe Martin <[email protected]>
Signed-off-by: Philippe Martin <[email protected]>
Signed-off-by: Philippe Martin <[email protected]>
Signed-off-by: Philippe Martin <[email protected]>
Signed-off-by: Philippe Martin <[email protected]>
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.
Signed-off-by: Philippe Martin <[email protected]>
Should be fixed with new commit. Thanks |
For now, I don't think that this pattern is used in other parts of Podman Desktop (for example, when searching containers with a specific term, going to a container details and coming back to the list cleans up the search term). |
That would need to change the look and feel of the Dropdown component, which is part of the Podman Desktop toolkit. I think it would be part of another issue.
It's not clear in the issue #1693, but as it is a separate behaviour, it would be better to implement it as a separate issue (or at least a different PR)
|
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.
LGTM, tested on Windows
On AI-Lab we have the ModelSelect example https://github.com/containers/podman-desktop-extension-ai-lab/blob/a3fbfb6a3d096824467a7007fd0bb06eceafa247/packages/frontend/src/lib/select/ModelSelect.svelte |
I usually don't expect to have a "No filter" in the select, but rather clearing the value, so no value selected should just be "no filter". The placeholder should not be "no filter" but "Filter on tool" or something helping the user to understand what this select is showing. I know those limitation are from the select we use from the ui library, that's why we can use the Svelte select we already use for the select model, and select container connection provider |
@MariaLeonova I'm working on a multi-select Dropdown on podman-desktop/podman-desktop#10621, feel free to test and comment there |
It seems that using a dropdown raises many technical and design problems. @MariaLeonova what do you think about a solution where the entire filter section would be visible or hidden, and with this design when the filter section is open (you can imagine a toggle button somewhere to completely close this section and just display a ![]() |
Can't we use the GitHub like synthax ? |
Yes, this point has been discussed in the issue. The conclusion has been to have both solutions, the dropdown/checkbox helping the user discover/learn the search syntax (as the tabs are an alternative of the search in the containers list) The search will be part of another PR |
That sound fine, I just have a couple questions: |
Yes, the checkboxes displayed in the mockup are intended to be checked/unchecked by the user.
Yes, a search bar will be present above the filters, and its content will be updated reactively according to the checkboxes checked or not. |
In order to simplify and move forward on a first version, I think the proposed solution here is working nicely: A couple of points:
We can create a separate issue for the multi-select and see if it's getting asked by the end-users. |
Signed-off-by: Philippe Martin <[email protected]>
Signed-off-by: Philippe Martin <[email protected]>
Signed-off-by: Philippe Martin <[email protected]>
Signed-off-by: Philippe Martin <[email protected]>
Added a few commits and updated the demo on top (#2354 (comment)) with these points |
Thanks @feloy ! The screen recording in the description of the PR is looking great! works fine for me ! |
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.
LGTM.
Minor comment: I see vectordb as a framework which seems a little weird for me I think wer should see the vectordb provider (chromadb, milvus). But should be a follow up PR and will open an issue
What does this PR do?
Filter recipes in the frontend
Limitation: we don't have a UI component to select multiple values for filters, this first implementation supports to select only one filter per category. @slemeur @Firewall @MariaLeonova is it acceptable for a first iteration, the time to implement a multi-select Dropdown?
Screenshot / video of UI
What issues does this PR fix or reference?
Fixes #1693
How to test this PR?
Go the the Recipe Catalog page, select different filters and check that the displayed recipes are correct, and that the choices proposed to the user after changing filters is also correct.