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

7 create dashboard #59

Closed
wants to merge 23 commits into from
Closed

7 create dashboard #59

wants to merge 23 commits into from

Conversation

SverreNystad
Copy link
Collaborator

No description provided.

{status === 'success' ? (
<div className="flex items-center text-green-600" data-test="status-indicator">
<svg
xmlns="http://www.w3.org/2000/svg"
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Is it possible to use one of the svgs in the public folder or react icon

Copy link
Collaborator

Choose a reason for hiding this comment

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

+1

// Simulate clicking the component and ensure it navigates to the detailed page
cy.get("[data-test=archiveGPT-component]")
.click()
.location("pathname") // TODO: Add route to report page
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

todo

Comment on lines +202 to +210
it("The page must ", () => {
it("should load the title", () => {
cy.visit(BASE_URL);
cy.get('[data-cy="title"]')
.should("exist")
.and("contain.text", "Plansituasjon:")
.and("be.visible");
});
});
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is the "The page must" wrapper needed here?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Fix double wrap here

Comment on lines 157 to 161
{/*
<div>
<ResultAI title={"ArkivGPT"} status={'success'} feedback={"Arkivdata funnet"} reportUrl={'https://www.youtube.com/watch?v=dQw4w9WgXcQ'} />
</div>
*/}
Copy link
Collaborator

@andreaslhjulstad andreaslhjulstad Oct 29, 2024

Choose a reason for hiding this comment

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

🗑️

Copy link
Collaborator

Choose a reason for hiding this comment

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

Is included in the newest pull request

}

const ResultAI: React.FC<StatusCardProps> = ({ title, status, feedback, reportUrl }) => {
const navigate = useNavigate();
Copy link
Collaborator

Choose a reason for hiding this comment

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

We don't use react router. Use useRouter() from next/navigation instead

Copy link
Collaborator

Choose a reason for hiding this comment

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

Already corrected in the newest pull request

}`}
onClick={() => onToggleSubItem(fileName, subItem.id)}
>
{subItem.isComplete ? '✔️' : '⭕'}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think react icons would look better

Copy link
Collaborator

Choose a reason for hiding this comment

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

Todo: Fix this

Comment on lines +21 to +28
alert(
`Status: ${status}\nFeedback: ${feedbackText}\nChecklist: ${JSON.stringify(
checklistSummary,
null,
2
)}`
);
};
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should probably be a custom modal component instead of an alert in the final product

Copy link
Collaborator

Choose a reason for hiding this comment

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

TODO: Make custom modal component

@@ -38,6 +38,7 @@
"ntnu-kpro-ai-assistant": "file:",
"react": "^18.3.1",
"react-dom": "^18.3.1",
"react-router-dom": "^6.27.0",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Remove this dependency and use next/navigations built-in routing

Copy link
Collaborator

Choose a reason for hiding this comment

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

TODO: Remove react-router

@@ -1,14 +1,170 @@
"use client";

import { useParams } from "next/navigation";
import { getCase, CaseData } from "~/types/cases";
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
import { getCase, CaseData } from "~/types/cases";
import { getCase, type CaseData } from "~/types/cases";

Copy link
Collaborator

Choose a reason for hiding this comment

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

TODO: Fix this

import Summary from "~/components/Summary";
import EmbeddedFrame from "~/components/EmbeddedFrame";
import CaseDocumentsComponent from "~/components/CaseDocuments";
import ResultAI from "~/components/ResultAI";
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is currently unused. Either use it or remove the import

Copy link
Collaborator

Choose a reason for hiding this comment

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

All imports are in use

import React from "react";
import { Detection } from "~/types/detection";
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
import { Detection } from "~/types/detection";
import type { Detection } from "~/types/detection";

Copy link
Collaborator

Choose a reason for hiding this comment

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

TODO: Fix this

const checklist = transformDetectionToChecklist(detections);

/** Convert from string to number for getCase function */
const caseNumberAsNumber = Number(caseNumber);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Also unused

Copy link
Collaborator

Choose a reason for hiding this comment

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

TODO: Fetch case number from URL

@@ -0,0 +1,81 @@
import React, { useState } from 'react';
import { ChecklistItemData } from './Checklist';
Copy link
Collaborator

@andreaslhjulstad andreaslhjulstad Oct 29, 2024

Choose a reason for hiding this comment

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

Suggested change
import { ChecklistItemData } from './Checklist';
import type { ChecklistItemData } from './Checklist';

Copy link
Collaborator

Choose a reason for hiding this comment

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

TODO: Fix this

Copy link
Collaborator

Choose a reason for hiding this comment

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

Formatting should be 2 spaces for consistency

Copy link
Collaborator

Choose a reason for hiding this comment

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

TODO: Review this

@@ -34,3 +36,43 @@ export const hasErrors = (result: Detection): boolean => {
export const capitalize = (str: string): string => {
return str.charAt(0).toUpperCase() + str.slice(1);
};

export const transformDetectionToChecklist = (
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice solution!

Copy link
Collaborator

Choose a reason for hiding this comment

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

We should probably find a better solution than copy-pasting the same file three different places. But ok for now

Copy link
Collaborator

Choose a reason for hiding this comment

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

TODO: Discuss in the group, don't wanna mess up routing

Copy link
Collaborator

Choose a reason for hiding this comment

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

Fix after user tests

Copy link
Collaborator

@andreaslhjulstad andreaslhjulstad left a comment

Choose a reason for hiding this comment

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

A couple small changes needed, but it was hard to find things to comment on.

Overall very good work!

@johanneeo johanneeo closed this Oct 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants