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

Build Out Storybook #230

Merged
merged 33 commits into from
Jul 2, 2024
Merged

Build Out Storybook #230

merged 33 commits into from
Jul 2, 2024

Conversation

4eyes52
Copy link

@4eyes52 4eyes52 commented Jun 8, 2024

Whats In this PR: I created stories for components that live in apps/ui but only for those components that are currently being used in /apps/nextjs/src/app/. I didn't think it was necessary to add stories for the ui components that weren't being used in the nextjs app. But please let me know if you'd like me to do that.

Gotchas:

  1. I added h-full and w-full classes added to carousel img in apps/ui/src/components/ui/carousel.tsx
    see comments: https://github.com/BibliothecaDAO/RealmsWorld/pull/230/files#r1640062467
  2. Variant declaration ghost prior to render was removed in apps/ui/src/components/ui/nav-link.tsx
    see comments: https://github.com/BibliothecaDAO/RealmsWorld/pull/230/files#r1640066436
  3. pb-4 class was removed from apps/ui/src/components/ui/nav-link.tsx
    see comments: https://github.com/BibliothecaDAO/RealmsWorld/pull/230/files#r1640069778

TODOs: I did not attempt to build these changes for production. I figured this PR was already getting pretty big so I thought just making sure storybook was building locally would be enough for now. I am very much willing to try building for production if there is a desire to do so.

Thank you for taking the time if anyone ends up reviewing these changes.

4eyes52 added 11 commits June 5, 2024 22:15
…ent, use new statusdot component in gamecard and games page
…e checkbox stories file since only instance of checkbox used is imported from 3rd party lib. delete variant = 'ghost' declaration from navlink in order to render diferent variations in storybook
…d stories for nav menu, popover,progress, scrollarea, sheet, switch, tabs and tooltip
Copy link

vercel bot commented Jun 8, 2024

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

Name Status Preview Comments Updated (UTC)
frontinus-house ❌ Failed (Inspect) Jun 10, 2024 10:18pm
realms-world-nextjs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jun 10, 2024 10:18pm

Copy link

vercel bot commented Jun 8, 2024

@4eyes52 is attempting to deploy a commit to the Loot Bibliotheca Team on Vercel.

A member of the Team first needs to authorize it.

@4eyes52 4eyes52 changed the base branch from main to dev June 12, 2024 12:06
@4eyes52
Copy link
Author

4eyes52 commented Jun 12, 2024

@RedBeardEth Im going to change the status of the PR to draft. I'm almost done just need to add a couple more components to storybook that are new in the dev branch. I'll switch back to ready for review when finished.

@4eyes52 4eyes52 marked this pull request as draft June 12, 2024 20:43
@@ -122,7 +122,7 @@ export function Carousel({
alt={image.alt}
//fill
sizes="100vw"
className={`${cover ? "object-cover" : "object-contain"}`}
className={`${cover ? "object-cover" : "object-contain"} h-full w-full`}
Copy link
Author

@4eyes52 4eyes52 Jun 14, 2024

Choose a reason for hiding this comment

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

I added h-full and w-full classes here so that the images would fit the carousel's container. I noticed that the Image component from next is not being imported anymore and a is being used instead. Next's image has a fill property that I think can be used in place of h-full w-full if you plan on using next's component instead.

Comment on lines -37 to -38

variant = "ghost";
Copy link
Author

Choose a reason for hiding this comment

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

I removed this variant declaration because it was causing storybook to render all the storieswith the ghost variant instead of the variant name I was trying to pass to args. I don't think this will cause issues in the next app but wanted to note.

return (
<Link
className={cn(
buttonVariants({ variant, size, className }),
"pb-4",
Copy link
Author

Choose a reason for hiding this comment

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

this pb-4 class was adding extra padding below the text of links. I removed it because it was messing up the layout of content provided to links in stories.

@4eyes52 4eyes52 marked this pull request as ready for review June 14, 2024 16:41
@4eyes52 4eyes52 requested a review from RedBeardEth June 14, 2024 16:41
Comment on lines +257 to +268
<tbody>
{tableData.map((data, index) => (
<tr key={index}>
<td className="whitespace-nowrap px-2 py-2 font-sans text-bright-yellow/70">
{data.key}
</td>
<td className="whitespace-nowrap px-2 py-2 text-right">
{data.value}
</td>
</tr>
))}
</tbody>
Copy link
Author

Choose a reason for hiding this comment

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

visiting a game page locally threw a run time error because tbody wasn't present

@RedBeardEth RedBeardEth merged commit 8441642 into BibliothecaDAO:dev Jul 2, 2024
0 of 4 checks passed
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.

None yet

2 participants