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

feat: create colony cards for landing page #3425

Open
wants to merge 3 commits into
base: feat/landing-page
Choose a base branch
from

Conversation

adam-strzelec
Copy link
Contributor

@adam-strzelec adam-strzelec commented Oct 22, 2024

Description

Create colony cards in all variants for landing page

Testing

  • Run storybook
  • Open Colony Card, Create New Colony Card, Colony Card Skeleton

Diffs

New stuff

Screenshot 2024-10-22 at 11 52 25

Main issue - #3392

@adam-strzelec adam-strzelec self-assigned this Oct 22, 2024
@adam-strzelec adam-strzelec requested review from a team as code owners October 22, 2024 10:16
@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.


strzelec seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

@adam-strzelec adam-strzelec changed the base branch from master to feat/landing-page October 22, 2024 10:26
Copy link
Contributor

@bassgeta bassgeta left a comment

Choose a reason for hiding this comment

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

Nice start on this, design wise stuff is looking quite on point 👌
However, I think we could benefit from some more splitting of components according to what they do, I've left a comment regarding that, feel free to reach out if I wasn't clear enough!
But essentially, I think we shouldn't have all of our new components receive a loading flag and render everything wrapped in LoadingSkeleton components.
It would be much more concise and readable to handle loading in a different component

import Button from '~v5/shared/Button/Button.tsx';

export interface ColonyCardProps {
colonyName?: string;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think all of the conditional rendering we're doing with skeletons and handling the create new colony function is putting way too many things into this component.
I would recommend the following steps to make this more sustainable:

  1. Create 2 new components, a ColonyCard and CreateNewColonyCard
  2. Instead of having all the props optional because the app may be loading, I would recommend creating a file structure something like this:
  • ColonyCardHandler is a component which gets the colony and the loading state, if the app is loading it renders ColonyCardSkeleton, if we have all the necessary data, we render ColonyCard with all the props sent in defined and not nullable
  • then the same for CreateColonyCard (can this one even be impacted by anything loading?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bassgeta Ok, I will add ColonyCardHandler when I start creating logic for Landing page

Copy link
Contributor

@mmioana mmioana left a comment

Choose a reason for hiding this comment

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

Nice work so far on this one @adam-strzelec 👏

All 3 scenarios for the cards look good, but please check @bassgeta and my comments for refactoring the ColonyCard component as it will make it easier to maintain and add more types of cards in the future

Screenshot 2024-10-23 at 09 58 38
Screenshot 2024-10-23 at 09 58 45
Screenshot 2024-10-23 at 09 58 54

</p>
) : (
<Button icon={Plus} onClick={onCreate}>
Create
Copy link
Contributor

Choose a reason for hiding this comment

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

Please extract this into a MessageDescriptor and then use here formatText(MSG.createColony)

Copy link
Contributor

Choose a reason for hiding this comment

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

Given we have multiple scenarios for the ColonyCard, namely the loading state, creating a colony and active colony, we should definitely create multiple instances of the ColonyCard and use them accordingly: LoadingColonyCard, CreateColonyCard and ActiveColonyCard - the last two could instantiate a BaseColonyCard which could have the following implementation

interface BaseColonyCardProps extends PropsWithChildren {
  isClickable?: boolean
}

const BaseColonyCard: FC<BaseColonyCardProps> = ({ children, isClickable }) => {

  return (
    <div
      className={clsx(
        'flex h-[4.5rem] items-center gap-[.875rem] rounded border px-5 py-4 transition-colors duration-normal hover:border-gray-900',
        {
          'cursor-pointer': isClickable,
        },
      )}
    >
      {children}
    </div>
  )
}

Copy link
Contributor

@bassgeta bassgeta left a comment

Choose a reason for hiding this comment

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

Lightspeed turnaround here, much appreciated 👌
All the components look good, and my oh my this composition is truly sweet to look at, really great job!
The only reason I'm requesting changes is because I would like to see us pulling all the i18n strings into the components that use them, for example

After this is resolved, I will happily approve this :D

Copy link
Contributor

@bassgeta bassgeta left a comment

Choose a reason for hiding this comment

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

🔥 looking good, thanks for the changes and bringing the landing page one step closer towards completion!

Copy link
Contributor

@mmioana mmioana left a comment

Choose a reason for hiding this comment

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

Way more composable implementation of these cards 🙌 thanks so much for your fast changes @adam-strzelec 💯

Screenshot 2024-10-24 at 10 35 54
Screenshot 2024-10-24 at 10 36 02
Screenshot 2024-10-24 at 10 36 09

Everything looks nice and will approve your PR 👍
However a question is popping in my mind if it wouldn't be better to structure all these variants under a single storybook Story as in the end they should only represent a variant of a component?

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.

4 participants