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

Evaluation results dashboard #170

Merged
merged 1 commit into from
Sep 16, 2024
Merged

Evaluation results dashboard #170

merged 1 commit into from
Sep 16, 2024

Conversation

csansoon
Copy link
Contributor

image

TRENECITO: Graphs will be added in a next PR.

Also missing: evaluationResults.providerLogId currently points to the last provider log of the document log that is being evaluated. Instead, this should point to the provider log that has been generated through the evaluation, the one that sets the value of evaluationResults.result

@csansoon csansoon added the 🚂 lil train Implementation divided in several feature-specific PRs label Sep 13, 2024
@csansoon csansoon force-pushed the evaluation-results-dashboard branch from a7de50b to 5ddedf8 Compare September 13, 2024 14:49
@csansoon csansoon marked this pull request as ready for review September 13, 2024 14:54
children?: ReactNode
}) {
const panel = (
<div className='min-w-44 flex-1 h-[84px] flex flex-col gap-1 p-4 rounded-lg border border-border'>
Copy link
Contributor

Choose a reason for hiding this comment

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

Try to avoid hardcoding all these values for width and height

<Text.H3B>
{mostCommonResult}{' '}
<Text.H3 color='foregroundMuted'>({resultPresence} %)</Text.H3>
</Text.H3B>
Copy link
Contributor

Choose a reason for hiding this comment

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

This is weird no? I text wrapping a text

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This way I was avoiding using two text components under a <>{…}</>. It's the same thing anyways, I'll use this instead.

<div className='flex gap-6 flex-wrap'>
<div className='h-[192px] min-w-[300px] flex-1 bg-muted-foreground rounded-lg' />
<div className='h-[192px] min-w-[300px] flex-1 bg-muted-foreground rounded-lg' />
<div className='min-w-[400px] flex-1'>
Copy link
Contributor

Choose a reason for hiding this comment

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

More hardcoded size

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In this case this is a placeholder while the actual charts are being developed.

table={<></>}
/>

<MetricsSummary
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not inside table from TableWithHeader?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

idk, feels misleading using the table attribute for non-table components...

andresgutgon
andresgutgon previously approved these changes Sep 16, 2024
export const getEvaluationByIdCached = cache(async (id: number) => {
const { workspace } = await getCurrentUser()
const evaluationScope = new EvaluationsRepository(workspace.id)
const result = await evaluationScope.findById(id)
Copy link
Collaborator

Choose a reason for hiding this comment

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

all repos implement find method which is == to this findById so we can probably remove this one

geclos
geclos previously approved these changes Sep 16, 2024
Copy link
Collaborator

@geclos geclos left a comment

Choose a reason for hiding this comment

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

missing tests but let's optimize for merging this asap and adding tests after the fact

@csansoon csansoon dismissed stale reviews from geclos and andresgutgon via 0c3c312 September 16, 2024 10:07
@csansoon csansoon force-pushed the evaluation-results-dashboard branch 4 times, most recently from 0e3ed2f to 9b8ff60 Compare September 16, 2024 10:21
@csansoon csansoon force-pushed the evaluation-results-dashboard branch from 9b8ff60 to ebdf5f8 Compare September 16, 2024 10:25
@csansoon csansoon merged commit 2c8576e into main Sep 16, 2024
2 of 3 checks passed
@csansoon csansoon deleted the evaluation-results-dashboard branch September 16, 2024 10:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🚂 lil train Implementation divided in several feature-specific PRs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants