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

[ref] component result card #76

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

Conversation

Joel-leal
Copy link
Contributor

Description

Result card component created and rendered on the results page.

Changes

  • Result card component created
  • Renders the list of candidates using the result card on the election results page.
Screenshot 2024-11-01 at 00 08 48

Board issue

Copy link

vercel bot commented Nov 1, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Skipped Deployment
Name Status Preview Comments Updated (UTC)
voting-system ⬜️ Ignored (Inspect) Dec 1, 2024 0:19am

@Joel-leal Joel-leal requested a review from frattezi November 1, 2024 03:09
@Joel-leal Joel-leal self-assigned this Nov 1, 2024
@Joel-leal Joel-leal added the enhancement New feature or request label Nov 1, 2024
@Joel-leal Joel-leal requested a review from marco-souza November 1, 2024 11:47

Choose a reason for hiding this comment

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

For an example application, it's ok to put images in a git repo, but for real applications, we would need a storage service (block storage, S3, and others)

Copy link
Contributor

Choose a reason for hiding this comment

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

I have an idea for a cool backlog, thank you for pointing that out!

Copy link
Contributor

Choose a reason for hiding this comment

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

@Joel-leal for now. Remove the images from the repo, we don't want these assets here. Find some public image on the internet (use this https://picsum.photos/) and use it while our data is mocked please.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok!

@@ -1,8 +1,26 @@
import ResultCard from "@packages/components/ResultCard";
import candidates from "src/data/data";

Choose a reason for hiding this comment

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

instead of getting the data directly, I'd create a DAO for candidates, abstracting the data access.

Therefore, when you switch your data source to a real DB, you won't need to change the data access interface

Copy link
Contributor

Choose a reason for hiding this comment

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

agreed

Copy link
Contributor

Choose a reason for hiding this comment

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

@Joel-leal please build this and mock until DAO level

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am going to study this DAO-level topic and implement it.

@@ -0,0 +1,50 @@
"use client";

Choose a reason for hiding this comment

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

why do we need this?

Comment on lines +35 to +48
const AvatarFallback = React.forwardRef<
React.ElementRef<typeof AvatarPrimitive.Fallback>,
React.ComponentPropsWithoutRef<typeof AvatarPrimitive.Fallback>
>(({ className, ...props }, ref) => (
<AvatarPrimitive.Fallback
ref={ref}
className={cn(
"flex h-full w-full items-center justify-center rounded-full bg-muted",
className,
)}
{...props}
/>
));
AvatarFallback.displayName = AvatarPrimitive.Fallback.displayName;

Choose a reason for hiding this comment

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

I personally found this code kind of ugly, code :(

Maybe we can use something like: https://www.npmjs.com/package/tailwind-styled-components

Choose a reason for hiding this comment

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

same issue for the shared components bellow

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

That's what shadcn components looks like. Then maybe choose something else? or just create the components from scratch?

Copy link
Contributor

Choose a reason for hiding this comment

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

Exactly, this is also code that we're not suposed to maintain, as zenha mentioned, this is shadcn's component, I don't see value on re-writing it.

Copy link

@marco-souza marco-souza Nov 5, 2024

Choose a reason for hiding this comment

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

this is so sad I wanna die 😢

Warning

I don't like shadcn

Copy link

@marco-souza marco-souza Nov 5, 2024

Choose a reason for hiding this comment

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

Let me explain better my POV:

How can we see a displayName and think this is ok?

We intended to share some styles, but instead we created forwardRef components that feature complex and poorly readable types, utilizing spread operators - all of that do add some custom CSS 🤯

In my opinion, this code is challenging to maintain because it requires a deep understanding of the need for forwardRef to customize shadcn components - not easy for a fresh junior or a 5y person to maintain

Copy link

@marco-souza marco-souza Nov 5, 2024

Choose a reason for hiding this comment

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

If you guys are saying that this is the shadcn way, I won't block this PR, BUT still ugly as hell

Let's at least create a wrapper to hide this ugliness

Copy link

@marco-souza marco-souza Nov 5, 2024

Choose a reason for hiding this comment

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

Suggested style wrapper WithStyles:

Suggested change
const AvatarFallback = React.forwardRef<
React.ElementRef<typeof AvatarPrimitive.Fallback>,
React.ComponentPropsWithoutRef<typeof AvatarPrimitive.Fallback>
>(({ className, ...props }, ref) => (
<AvatarPrimitive.Fallback
ref={ref}
className={cn(
"flex h-full w-full items-center justify-center rounded-full bg-muted",
className,
)}
{...props}
/>
));
AvatarFallback.displayName = AvatarPrimitive.Fallback.displayName;
function WithStyles(ShadcnElement, styles) {
const CustomComponent = React.forwardRef<
React.ElementRef<typeof ShadcnElement>,
React.ComponentPropsWithoutRef<typeof ShadcnElement>
>(({ className, ...props }, ref) => (
<ShadcnElement
ref={ref}
className={cn(styles, className)}
{...props}
/>
));
CustomComponent.displayName = ShadcnElement.displayName;
return CustomComponent;
}

Then you can do something like

const AvatarFallback = WithStyles(
  AvatarPrimitive.Fallback,
  "flex h-full w-full items-center justify-center rounded-full bg-muted",
)

const Avatar = WithStyles(
  AvatarPrimitive.Root,
  "flex h-full w-full items-center justify-center rounded-full bg-muted",
)

const AvatarImage = WithStyles(
  AvatarPrimitive.Image,
  "flex h-full w-full items-center justify-center rounded-full bg-muted",
)

Copy link
Contributor

@frattezi frattezi left a comment

Choose a reason for hiding this comment

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

Great work my friend! Added some comments alongside with the guys! Let me know if you need any help

image,
}: ElectionResult) {
return (
<Card className="flex flex-row w-[60vw] p-4 shadow-md shadow-gray-200 border-[rgba(0,0,0,0)]">
Copy link
Contributor

Choose a reason for hiding this comment

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

Is [rgba(0,0,0,0)] necessary? border-black wouldn't work?

candidate,
vice,
party,
percentagem,
Copy link
Contributor

Choose a reason for hiding this comment

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

Variable in portuguese

vice,
party,
percentagem,
votos,
Copy link
Contributor

Choose a reason for hiding this comment

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

same as above

<AvatarImage src={image} />
</Avatar>
<div>
<p className="font-bold">Presidente: {candidate}</p>
Copy link
Contributor

Choose a reason for hiding this comment

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

It's ok to have portuguese on the text displayed to the user (we can enhance that with internationalization) but code and variables, always in colonizador language

@@ -1,8 +1,26 @@
import ResultCard from "@packages/components/ResultCard";
import candidates from "src/data/data";
Copy link
Contributor

Choose a reason for hiding this comment

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

@Joel-leal please build this and mock until DAO level

@@ -4,7 +4,7 @@ generator client {

datasource db {
provider = "postgresql"
url = env("DATABASE_URL")
url = "postgres://test:test@localhost:5432/test"

Choose a reason for hiding this comment

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

(╯°□°)╯︵ ┻━┻ 💥

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is a bug that we still don't understand, but there is a local error that doesn't accept this reference. However, on the remote side, it is accepted. I will push it corrected, and then I will investigate again to find out what might be causing it. Another sticker unlocked!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Create component ResultCard
4 participants