refactor: online sync plans code refactor & small features #114
Conversation
qamarq
changed the title
There was a problem hiding this comment.
Copilot reviewed 6 out of 18 changed files in this pull request and generated no suggestions.
Files not reviewed (12)
- frontend/package-lock.json: Language not supported
- frontend/package.json: Language not supported
- frontend/src/app/plans/edit/[id]/page.client.tsx: Evaluated as low risk
- frontend/src/app/plans/edit/[id]/page.tsx: Evaluated as low risk
- frontend/src/app/plans/page.client.tsx: Evaluated as low risk
- frontend/src/actions/plans.ts: Evaluated as low risk
- frontend/src/app/plans/page.tsx: Evaluated as low risk
- frontend/src/app/layout.tsx: Evaluated as low risk
- frontend/src/lib/usePlan.ts: Evaluated as low risk
- frontend/src/components/PlanDisplayLink.tsx: Evaluated as low risk
- frontend/src/app/plans/edit/[id]/_components/SyncedButton.tsx: Evaluated as low risk
- frontend/src/app/plans/edit/[id]/_components/OfflineAlert.tsx: Evaluated as low risk
| const message = useMemo(() => { | ||
| if (synced && equalsDates) { | ||
| return "Zsynchronizowano"; | ||
| } else if (!(onlineId ?? "")) { | ||
| return "Plan dostępny tylko lokalnie"; | ||
| } else if (syncing) { | ||
| return "Synchronizowanie..."; | ||
| } else if (!equalsDates) { | ||
| return "Twoja wersja różni się od wersji online"; | ||
| } | ||
| return "Masz lokalne zmiany"; | ||
| }, [synced, onlineId, syncing, equalsDates]); |
There was a problem hiding this comment.
zdublowałeś tą logike, jest tutaj i niżej decyduje jaki komponent wyświetlić, wydziel to do jednej funkcji bo to ohydny kod i jestem pewien, że da się go zrobić lepiej
| useEffect(() => { | ||
| resetInactivityTimer(); | ||
| return () => { | ||
| if (inactivityTimeout.current !== null) { | ||
| clearTimeout(inactivityTimeout.current); | ||
| } | ||
| }; | ||
| }, [plan.name, plan.courses, plan.registrations, plan.allGroups]); |
There was a problem hiding this comment.
nie rozumiem trochę po co refa tutaj użyłeś zamiast wszystko wrzucić do środka tego useEffecta
| <Tooltip> | ||
| <TooltipTrigger asChild={true}> | ||
| <Button asChild={true} size="icon"> | ||
| <Link href={`/plans/preview/${id}`}> | ||
| <FolderSearch className="size-4" /> | ||
| </Link> | ||
| </Button> | ||
| </TooltipTrigger> | ||
| <TooltipContent> | ||
| <p>Podgląd</p> | ||
| </TooltipContent> | ||
| </Tooltip> |
There was a problem hiding this comment.
pamiętaj, że na telefonie tooltipy nie działają, moze zamiast ikonki to napis z ikonką?
There was a problem hiding this comment.
Czemu? Przecież jak przytrzymasz to się pojawia. Plus właśnie dlatego dałem ikonki bo za dużo miejsca, chyba że tylko ja tel pokazywać tekst. Wgl zacznijmy od tego że on nie jest bardzo bardzo potrzebny i to bardziej dodatek zważywszy że większość ludzi na kompie wchodzi na to bo wygodniej
| plan: PlanState, | ||
| refetchOnlinePlan: () => Promise<T>, | ||
| setSyncing: (value: boolean) => void, |
There was a problem hiding this comment.
jak już masz tyle argumentów to przekazuj obiekt
| plan: PlanState, | |
| refetchOnlinePlan: () => Promise<T>, | |
| setSyncing: (value: boolean) => void, | |
| {plan, refetchOnlinePlan, setSyncing}:{plan: PlanState, | |
| refetchOnlinePlan: () => Promise<T>, | |
| setSyncing: (value: boolean) => void} |
(to nie zmienia faktu, że z tej abstrakcji aż cieknie)
| <CloudIcon className="size-4 text-emerald-500" /> | ||
| ) : !(onlineId ?? "") ? ( | ||
| <AlertTriangleIcon className="size-4 text-rose-500" /> | ||
| ) : syncing ? ( |
There was a problem hiding this comment.
Nie można tutaj jakoś ładniej tego zrobić? Bo te ternary w renderze trochę średnio wyglądają
| className="min-w-10" | ||
| onClick={() => { | ||
| if (!equalsDates) { | ||
| bounceAlert(); |
| import { useAtom } from "jotai"; | ||
|
|
||
| import { type ExtendedCourse, planFamily } from "@/atoms/planFamily"; | ||
|
|
||
| import type { Registration } from "./types"; | ||
|
|
||
| export interface PlanState { |
There was a problem hiding this comment.
Nie chcemy wszystkich interfejsów razem trzymać w types?
There was a problem hiding this comment.
chcemy, lint bartusia nie chce
| message: string; | ||
| } | ||
|
|
||
| export type CourseType = Array<{ |
There was a problem hiding this comment.
Pewnie przekopiowałeś to, ale nie możemy zrobić wszędzie interfejsów, żeby spójnie było?
There was a problem hiding this comment.
nie da się takiego typu zrobić interfacem niestety
This pull request includes various updates and improvements to the frontend codebase, particularly involving the addition of a new tooltip component and the refactoring of plan-related types and functions. The most important changes include adding the
@radix-ui/react-tooltippackage, refactoring type imports, and enhancing the user interface with tooltips and offline alerts.Package Updates:
@radix-ui/react-tooltipversion1.1.4topackage.jsonandpackage-lock.json. [1] [2] [3]Type Refactoring:
frontend/src/actions/plans.tsto use a centralized types file.UI Enhancements:
TooltipProviderto the root layout infrontend/src/app/layout.tsxto enable tooltips across the application. [1] [2] [3]SyncedButtoncomponent to provide additional context to users. (frontend/src/app/plans/edit/[id]/_components/SyncedButton.tsxL10-R17, frontend/src/app/plans/edit/[id]/_components/SyncedButton.tsxR26-R59, frontend/src/app/plans/edit/[id]/_components/SyncedButton.tsxR77-R81)OfflineAlertcomponent to notify users when they are offline. (frontend/src/app/plans/edit/[id]/_components/OfflineAlert.tsxR1-R12)Code Organization:
CreateNewPlanPagetopage.client.tsxand updated imports accordingly. (frontend/src/app/plans/edit/[id]/page.client.tsxL4-R12, frontend/src/app/plans/edit/[id]/page.client.tsxR32-R41, frontend/src/app/plans/edit/[id]/page.tsxL5-R5)