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.

Sign up for GitHub

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

Jump to bottom

(frontend)[AGE-1393]: Update create new evaluation modal with new design #2337

Merged
merged 22 commits into from
Dec 9, 2024
Merged

(frontend)[AGE-1393]: Update create new evaluation modal with new design #2337

merged 22 commits into from
Dec 9, 2024

Conversation

bekossy
Copy link
Member

@bekossy bekossy commented Dec 4, 2024

Closes AGE-1393

@vercel Vercel
Copy link

vercel bot commented Dec 4, 2024 •
edited
Loading

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 Dec 9, 2024 2:33pm
agenta-documentation ✅ Ready (Inspect) Visit Preview 💬 Add feedback Dec 9, 2024 2:33pm

@bekossy bekossy marked this pull request as ready for review December 5, 2024 02:51
@dosubot dosubot bot added size:XXL This PR changes 1000+ lines, ignoring generated files. Frontend labels Dec 5, 2024
@mmabrouk mmabrouk enabled auto-merge December 5, 2024 16:15
mmabrouk
mmabrouk requested changes Dec 5, 2024
View reviewed changes
Copy link
Member

@mmabrouk mmabrouk left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks @bekossy

Here is my review:

  • Selecting the test case should collapse the first collapsible, since anyways the user can select only one
  • When you uncollapse any collapsible it uncollapses in two speeds. One along the table and a second speed for the empty space (see video)
  • Not sure about this one, please check with Ahmed, but if I have only variant why do I need so much empty space in the collapsible
    https://github.com/user-attachments/assets/594e0bed-977e-4a1d-a59e-61b121236f1f
  • Size don't match (see next comment)
  • Advanced config button update
  • E2E tests failing

@ardaerzin can you please review the code

@mmabrouk
Copy link
Member

mmabrouk commented Dec 5, 2024

CleanShot 2024-12-05 at 17 23 09@2x

ardaerzin
ardaerzin requested changes Dec 5, 2024
View reviewed changes
Copy link
Contributor

@ardaerzin ardaerzin left a comment

Choose a reason for hiding this comment

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

I've left a few comments. lmk if you have any questions

agenta-web/src/components/pages/evaluations/NewEvaluation/SelectEvaluatorSection.tsx Outdated Show resolved Hide resolved
agenta-web/src/components/pages/evaluations/NewEvaluation/SelectVariantSection.tsx Outdated Show resolved Hide resolved
agenta-web/src/components/pages/evaluations/NewEvaluation/SelectVariantSection.tsx Outdated Show resolved Hide resolved
agenta-web/src/components/pages/evaluations/NewEvaluation/SelectTestsetSection.tsx Outdated Show resolved Hide resolved
agenta-web/src/components/pages/evaluations/NewEvaluation/SelectEvaluatorSection.tsx Outdated Show resolved Hide resolved
@ardaerzin
Copy link
Contributor

@bekossy it feels a little weird to have 1 modal that closes when mask is clicked, next to another one that doesn't. I think we should keep the behavior consistent. I see the evaluators modal got that behavior disabled recently, might make sense to mimic that behavior in this one too

@bekossy bekossy marked this pull request as ready for review December 8, 2024 14:27
@bekossy bekossy requested review from ardaerzin and mmabrouk December 8, 2024 14:28
@mmabrouk
Copy link
Member

mmabrouk commented Dec 9, 2024

CleanShot 2024-12-09 at 13 37 22@2x

@bekossy lgtm, only minor comment. The model columns seems not to be working. Let's either get it to work or remove it. Other than that, ready to merge

ardaerzin
ardaerzin requested changes Dec 9, 2024
View reviewed changes
agenta-web/src/components/pages/evaluations/NewEvaluation/SelectVariantSection.tsx Outdated
{...props}
className={classes.collapseContainer}
activeKey={activePanel === "variantPanel" ? "variantPanel" : undefined}
onChange={() => handlePanelChange("variantPanel")}
items={[
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry if I wasn't clear on the first review. What I meant before was memoization of the items array that are passed to collapsed components. As we saw during the call, using that practice will also bring performance updates during collapse ui state changes. 🙏

@bekossy bekossy requested a review from ardaerzin December 9, 2024 14:29
ardaerzin
ardaerzin approved these changes Dec 9, 2024
View reviewed changes
Copy link
Contributor

@ardaerzin ardaerzin left a comment

Choose a reason for hiding this comment

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

Thank you @bekossy 🙏 looking good to me

mmabrouk
mmabrouk approved these changes Dec 9, 2024
View reviewed 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.

lgtm

@dosubot dosubot bot added the lgtm This PR has been approved by a maintainer label Dec 9, 2024
@mmabrouk mmabrouk merged commit 7f6f891 into main Dec 9, 2024
9 checks passed
@mmabrouk mmabrouk deleted the AGE-1393/-update-create-new-evaluation-modal-with-new-design branch December 9, 2024 14:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ardaerzin ardaerzin ardaerzin approved these changes

@mmabrouk mmabrouk mmabrouk approved these changes

Assignees
No one assigned
Labels
cypress Frontend lgtm This PR has been approved by a maintainer reactjs size:XXL This PR changes 1000+ lines, ignoring generated files. UI
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@bekossy @mmabrouk @ardaerzin @ashrafchowdury