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

Fix biome-ignore issues (#826) #972

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

shagunsharma6677
Copy link

@shagunsharma6677 shagunsharma6677 commented Jul 31, 2024

Type of change

  • 🐛 Bug fix
  • 🌟 New feature
  • 🔨 Breaking change
  • 📖 Refactoring / dependency upgrade / documentation

Description

Fixed biome-ignore issues related to lint suppression. The following issues have been addressed:

  • noArrayIndexKey: Used nanoId package for unique keys in looping elements (e.g., key={skeleton-loading-${nanoid(6)}}).
  • noUnusedVariables: Commented out unused variables and functions.
  • useSortedClasses: Added css merge util in packages/react to handle strings and variables in className (e.g., className={cn("relative inline-flex h-2 w-2 rounded-full", color)}).
  • noAssignInExpressions: Fixed issues by optimizing code.
  • noForEach: Replaced forEach with for loops or map.
  • noAccumulatingSpread: Used direct assignments (e.g., acc[key] = value; // Directly assign to the accumulator) instead of the spread operator (e.g., return { ...acc, [key]: value };).
  • noExplicitAny, noImplicitAnyLet: Created types AssertionType, StatusAssertionType, and TextBodyAssertionType.

Some issues related to // biome-ignore lint/correctness/useExhaustiveDependencies may cause side effects, so they were not changed. Remaining biome-ignore issues are:

  • React.useEffect(() => { setOpen(false); }, [pathname]); in the menu component (web)
  • useEffect(() => { if (searchParams.has("authorize")) { const authorize = searchParams.get("authorize"); if (!authorize) return; form.setValue("password", authorize); } setLoading(false); }, []); in the password form (web)
  • useEffect(() => { if (document) { const cookie = document.cookie.split("; ").find((row) => row.startsWith(name)); if (!cookie) { setState(defaultValue); return; } const value = cookie.split("=")[1] as T; setState(value); } }, []); in useCookieState (web)

A picture tells a thousand words (if any)

Changes have passed all biome-lint checks.

Screenshot from 2024-07-31 13-59-42

Before this PR

{Please add a screenshot here}

After this PR

{Please add a screenshot here}

Related Issue (optional)


Copy link

vercel bot commented Jul 31, 2024

@shagunsharma6677 is attempting to deploy a commit to the OpenStatus Team on Vercel.

A member of the Team first needs to authorize it.

@@ -2,6 +2,7 @@ import type { Monitor } from "@openstatus/tinybird";
import { Tracker as OSTracker, classNames } from "@openstatus/tracker";

import { cn, formatDate } from "@/lib/utils";
import { nanoid } from "nanoid";
Copy link
Member

Choose a reason for hiding this comment

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

are you sure about using nanoid?

Copy link
Author

Choose a reason for hiding this comment

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

Hey @thibaultleouay ,

Yes, I have done a bit of my research on using a production-grade way to use keys for React elements, and I found this nanoid package, which has over 37 million weekly downloads with a 10.9 KB unpacked size.

If you can suggest any better options, I am happy to work on that.

Screenshot from 2024-09-07 11-35-41

@@ -11,6 +11,9 @@ import {
CardTitle,
} from "@/components/marketing/card";

import { Globe } from "@/components/marketing/monitor/globe";
Copy link
Member

Choose a reason for hiding this comment

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

Why do. we import globe ?

Copy link
Author

Choose a reason for hiding this comment

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

The Globe component is imported but not used. I am remove it to clean up the codebase.

const segmentsWithoutRouteGroup = selectedSegments.filter(
(segment) => !segment.startsWith("("),
);
// The following function is commented out because it is not currently used in the codebase.
Copy link
Member

Choose a reason for hiding this comment

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

Let's remove it 🔥

Copy link
Author

Choose a reason for hiding this comment

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

Done

// biome-ignore lint/correctness/noUnusedVariables: <explanation>
function stringToArrayProcess<T>(_string: T) {}
// Below function is commented as not used to remove biome-ignore
// function stringToArrayProcess<T>(_string: T) {}
Copy link
Member

Choose a reason for hiding this comment

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

please remove the dead code 🔥

Copy link
Author

Choose a reason for hiding this comment

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

done

Copy link
Member

@thibaultleouay thibaultleouay left a comment

Choose a reason for hiding this comment

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

Thanks again for your huge PR

Im not sure about nanoid as key a part from that 🔥

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.

2 participants