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

Hide latency and cost columns on filter #1512

Merged
merged 5 commits into from
Apr 17, 2024

Conversation

bekossy
Copy link
Member

@bekossy bekossy commented Apr 14, 2024

Issue #1511

@dosubot dosubot bot added the size:XS This PR changes 0-9 lines, ignoring generated files. label Apr 14, 2024
Copy link

vercel bot commented Apr 14, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
agenta ✅ Ready (Inspect) Visit Preview 💬 Add feedback Apr 17, 2024 11:35am

Copy link
Member

@mmabrouk mmabrouk left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @bekossy
It does not work for me, see video

Screen.Recording.2024-04-14.at.11.45.41.mov

Also, can we not close down the menu for filtering each time we select/unselect a column? The right behavior is that the menu get closed if we 1) click somewhere else or 2) click on the menu again. Basically the user would open the menu, start selecting/unselecting column, and then when finished close the menu

Also, can we add a dropdown icon to the right of filer columns to make it clear that it is a drop down

@dosubot dosubot bot added size:S This PR changes 10-29 lines, ignoring generated files. and removed size:XS This PR changes 0-9 lines, ignoring generated files. labels Apr 14, 2024
@bekossy
Copy link
Member Author

bekossy commented Apr 14, 2024

Thanks for the review @mmabrouk , the PR was actually for a similar issue in the comparison view but thanks for spotting it in the Evaluation results view
I have updated the PR according to the requested changes and it is ready for review

Copy link
Member

@mmabrouk mmabrouk left a comment

Choose a reason for hiding this comment

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

Thanks @bekossy for the quick fix!

I was also under that impression when I read the description. But I don't see any multiselect for filtering column in the comparison view
Screenshot 2024-04-14 at 13 02 32

@dosubot dosubot bot added size:L This PR changes 100-499 lines, ignoring generated files. and removed size:S This PR changes 10-29 lines, ignoring generated files. labels Apr 16, 2024
@bekossy bekossy requested a review from mmabrouk April 16, 2024 14:11
Copy link
Member

@mmabrouk mmabrouk left a comment

Choose a reason for hiding this comment

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

Thanks for the fix @bekossy
However the new drop down is showing the list of apps and not the list of evaluators!
Screenshot 2024-04-16 at 16 12 30

Copy link
Member

@mmabrouk mmabrouk left a comment

Choose a reason for hiding this comment

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

Thanks for the change @bekossy !

Sorry for the back and forth. I should have written a proper issue with wireframes.

The multiselect for filtering should not filter all columns but only the evaluator
Screenshot 2024-04-17 at 09 47 01
So this should read:

  • Output
  • Exact Match
  • Latency
  • Cost

And when you select output, you will hide all output columns for all the selected variants (in the left part).

So let's say I make a comparison of four variants a,b,c,d with two evaluators e1 and e2.

In the beginning I would see 4x5 columns (we always have output, cost and latency in addition to e1 and e2). I would deselect in the left c and d and end up with 10 columns. Then I would deselect cost and latency, and end up with a output, b output, a e1, b e1, a e2, b e2.

Hope that makes sense

@bekossy
Copy link
Member Author

bekossy commented Apr 17, 2024

Thank you for the explanation @mmabrouk, I really appreciate!
I have updated the PR to meet the requested changes

Copy link
Member

@mmabrouk mmabrouk left a comment

Choose a reason for hiding this comment

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

Thanks a lot @bekossy !!

@dosubot dosubot bot added the lgtm This PR has been approved by a maintainer label Apr 17, 2024
@mmabrouk mmabrouk merged commit 4a4a45c into main Apr 17, 2024
8 checks passed
@mmabrouk mmabrouk deleted the issue-1511/-filter-latency-and-cost-in-comparison-view branch April 17, 2024 11:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request Frontend lgtm This PR has been approved by a maintainer reactjs size:L This PR changes 100-499 lines, ignoring generated files. typescript
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enhancement: Include Cost and Latency Filtering in Comparison View
2 participants