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

Implement Validation for Filtering #86

Merged
merged 15 commits into from
Oct 27, 2024
Merged

Implement Validation for Filtering #86

Changes from 3 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
358b5ad
feat: added validation
gpalmer27 Oct 8, 2024
9bcf55e
fix: added toast feature
gpalmer27 Oct 9, 2024
e04ce1b
fix: fixed formatting
gpalmer27 Oct 11, 2024
98605a7
fix: added more to toast
gpalmer27 Oct 25, 2024
8799929
feat: add toast
gpalmer27 Oct 25, 2024
2784465
merge main
gpalmer27 Oct 26, 2024
54c8201
fixed formatting
gpalmer27 Oct 26, 2024
b79bca6
deleted extra line
gpalmer27 Oct 26, 2024
c3c630d
fixed bug
gpalmer27 Oct 27, 2024
3de1b4e
fixed bug
gpalmer27 Oct 27, 2024
311f1a3
fixed bugs
gpalmer27 Oct 27, 2024
101d169
fixed bugs
gpalmer27 Oct 27, 2024
3c0ac8d
fixed bugs
gpalmer27 Oct 27, 2024
c5f5838
fixed style
gpalmer27 Oct 27, 2024
580e813
fixed formatting
gpalmer27 Oct 27, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
71 changes: 69 additions & 2 deletions apps/web/src/app/(pages)/(dashboard)/roles/page.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,12 @@
"use client";

import { useState } from "react";
import { useEffect, useState } from "react";

import type {
import {

Check warning on line 5 in apps/web/src/app/(pages)/(dashboard)/roles/page.tsx

View workflow job for this annotation

GitHub Actions / lint 

Imports "ReviewType", "WorkEnvironmentType" and "WorkTermType" are only used as type
Copy link
Collaborator

Choose a reason for hiding this comment

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

You can fix this warning by doing something like this:

import type {
  ReviewType,
  WorkEnvironmentType,
  WorkTermType,
} from "@cooper/db/schema";
import { WorkEnvironment, WorkTerm } from "@cooper/db/schema";

ReviewType,
WorkEnvironment,
WorkEnvironmentType,
WorkTerm,
WorkTermType,
} from "@cooper/db/schema";
import { cn } from "@cooper/ui";
Expand All @@ -22,6 +24,24 @@
workEnvironment?: WorkEnvironmentType;
};
}) {
const [error, setError] = useState<string | undefined>(undefined);

useEffect(() => {
Copy link
Collaborator

@RishikeshNK RishikeshNK Oct 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

We could get away without using a useEffect or possibly a useState for tracking the errors!

We can define a RolesSearchParam schema in the validators package using Zod. Could be something like the following:

const RolesSearchParam = z.object({
  cycle: z.nativeEnum(WorkTerm).optional(),
  term: z.nativeEnum(WorkEnvironment).optional(),
});

We can share this object between the web app and the list endpoint for reviews. For validation, we can do something like the following:

const validationResult = RolesSearchParam.safeParse(searchParams);

const reviews = api.review.list.useQuery({
  options: validationResult.success ? validationResult.data : {},
});

At the same time, we can also use the same success property to display the error modal / toast:

{!validationResult.success && (
        <ErrorAlert message="Invalid search parameters." />
      )}

If you want more specific error messages, you can modify the Zod schema like this:

const RolesSearchParam = z.object({
  cycle: z
    .nativeEnum(WorkTerm, {
      message: "Invalid cycle type",
    })
    .optional(),
  term: z
    .nativeEnum(WorkEnvironment, {
      message: "Invalid term type",
    })
    .optional(),
});

and get the errors using validationResult.error.

const isValidTerm = searchParams?.workTerm
Copy link
Collaborator

Choose a reason for hiding this comment

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

i'm fairly certain we don't need to check for random invalid search params, like if someone puts josh=best-pl-ever, we can safely ignore

Copy link
Collaborator

Choose a reason for hiding this comment

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

just a comment, not a change

? Object.values(WorkTerm).includes(searchParams.workTerm)
: true;

const isValidEnvironment = searchParams?.workEnvironment
? Object.values(WorkEnvironment).includes(searchParams.workEnvironment)
: true;

if (!isValidTerm) {
setError("Invalid work term.");
} else if (!isValidEnvironment) {
setError("Invalid work environment.");
}
}, [searchParams]);

const reviews = api.review.list.useQuery({
options: {
cycle: searchParams?.workTerm,
Expand All @@ -33,10 +53,57 @@
reviews.data ? reviews.data[0] : undefined,
);

const [showAlert, setShowAlert] = useState(true);

return (
<>
<SearchFilter />
{/* TODO: Loading animations */}
{error && showAlert && (
<div
Copy link
Collaborator

Choose a reason for hiding this comment

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

Totally optional but I'd recommend taking a look at shadcn/ui. Almost all of our reusable components are built on top of shadcn. They do have a toast that you can use!

If you're interested, check out this docs page. You can add the component from the root folder using pnpm ui-add toast and use that component here.

id="bad-input"
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 we should make this a component so we can reuse this toast pop up in other places, @RishikeshNK thoughts?

Copy link
Collaborator

@RishikeshNK RishikeshNK Oct 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Yup! I agree. We can move this to a separate component that takes in the message as a prop. @joshw2048

className="mb-4 flex w-full max-w-xs items-center rounded-lg bg-white p-4 text-gray-500 shadow dark:bg-gray-800 dark:text-gray-400"
role="alert"
>
<div className="inline-flex h-8 w-8 flex-shrink-0 items-center justify-center rounded-lg bg-red-100 text-red-500 dark:bg-red-800 dark:text-red-200">
<svg
className="h-5 w-5"
aria-hidden="true"
xmlns="http://www.w3.org/2000/svg"
fill="currentColor"
viewBox="0 0 20 20"
>
<path d="M10 .5a9.5 9.5 0 1 0 9.5 9.5A9.51 9.51 0 0 0 10 .5Zm3.707 11.793a1 1 0 1 1-1.414 1.414L10 11.414l-2.293 2.293a1 1 0 0 1-1.414-1.414L8.586 10 6.293 7.707a1 1 0 0 1 1.414-1.414L10 8.586l2.293-2.293a1 1 0 0 1 1.414 1.414L11.414 10l2.293 2.293Z" />
</svg>
<span className="sr-only">Error icon</span>
</div>
<div className="ms-3 text-sm font-normal">{error}</div>
<button
type="button"
className="-mx-1.5 -my-1.5 ms-auto inline-flex h-8 w-8 items-center justify-center rounded-lg bg-white p-1.5 text-gray-400 hover:bg-gray-100 hover:text-gray-900 focus:ring-2 focus:ring-gray-300 dark:bg-gray-800 dark:text-gray-500 dark:hover:bg-gray-700 dark:hover:text-white"
data-dismiss-target="#bad-input"
aria-label="Close"
onClick={() => setShowAlert(false)}
>
<span className="sr-only">Close</span>
<svg
className="h-3 w-3"
aria-hidden="true"
xmlns="http://www.w3.org/2000/svg"
fill="none"
viewBox="0 0 14 14"
>
<path
stroke="currentColor"
stroke-linecap="round"
stroke-linejoin="round"
stroke-width="2"
d="m1 1 6 6m0 0 6 6M7 7l6-6M7 7l-6 6"
/>
</svg>
</button>
</div>
)}
{reviews.data && (
<div className="mb-8 grid h-[75dvh] w-4/5 grid-cols-5 gap-4 lg:w-3/4">
<div className="col-span-2 gap-3 overflow-scroll pr-4">
Expand Down