Skip to content

Conversation

rin-st
Copy link
Member

@rin-st rin-st commented Feb 16, 2025

Initial PR for homepage migration

User is static (hardcoded) for now and some parts could not work yet because of that, will fix it later.

See also comments in the PR

Some questions:

image
  1. Do we need logo?
  2. Will we show every home/portfolio etc buttons on the header or make it as on current sre? (portfolio only)
image
  1. As I understand we don't need balance and network, right?

  2. Hide "view on block explorer" button?

  3. Do we need Eth price at the bottom?

Note: I can't add more than one reviewer
cc @technophile-04 @Pabl0cks

Fixes #8

@rin-st rin-st requested a review from carletex February 16, 2025 18:15
@technophile-04
Copy link
Member

Thanks @rin-st ! Just quickly went through it and I think we need handle max width in wide screens a bit properly, but also maybe we can handle in future PR probably

Current version :
Screenshot 2025-02-17 at 5 28 25 PM

SRE-1
image

Also in normal 14inch it looks like this, there is empty right space
Screenshot 2025-02-17 at 5 30 08 PM

Comment on lines 51 to 62
{challenges.slice(0, lastChallengeBeforeJoinBgIndex + 1).map((challenge, index, { length }) => (
<ChallengeExpandedCard
key={challenge.id}
challengeId={challenge.id}
challenge={challenge}
challengeIndex={challenge.sortOrder}
builderAttemptedChallenges={builderAttemptedChallenges}
// connectedBuilder={connectedBuilder}
isFirst={index === 0}
isLast={length - 1 === index}
/>
))}
Copy link
Member

Choose a reason for hiding this comment

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

I think we talked about making this less "dynamic", right?

So the ChallengeExpandedCard could be less abstract / less params

Copy link
Member Author

Choose a reason for hiding this comment

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

Could you add details to this, please?

As I remember the main reason for that was that we need to easy add something between challenge cards? So it's implemented by using index of the card near which we want to add new block.

But how to make the component less "dynamic"? You're not meaning you want to create separate component for every challenge, right?


note: I see here that I need to remove for example challengeId and challengeIndex props since I already pass challenge, but it's not related to "dynamic" part

Copy link
Member

Choose a reason for hiding this comment

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

I think what we talked is this:

<div>
  <ChallengeExpandedCard id="simple-nft" whatEverProps />
  <ChallengeExpandedCard id="..." whatEverProps />
  <ChallengeExpandedCard id="..." whatEverProps />
  <JoinBGBanner />
  /* What ever */
</div>

I think this give us more flexibility.... thinking about the future when we'll have a bunch of challenges (and we'll only display some of them on the home)

I was refering to the "map" as the "dynamic" part. There were some weird logic because of doing this (e.g. showing borders, etc)

Really open to this, just mentioning this because this is what we discussed on the call.

Copy link
Member Author

@rin-st rin-st Feb 18, 2025

Choose a reason for hiding this comment

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

Makes sense, thanks!

Now it works that way:

...
       <ChallengeExpandedCard
          key="minimum-viable-exchange"
          challengeId="minimum-viable-exchange"
          builderAttemptedChallenges={builderAttemptedChallenges}
        />

        <JoinBGCard builderAttemptedChallenges={builderAttemptedChallenges} connectedBuilder={connectedBuilder} />

        <ChallengeExpandedCard
          key="state-channels"
          challengeId="state-channels"
          builderAttemptedChallenges={builderAttemptedChallenges}
        />
...

regarding builderAttemptedChallenges it's not final, I think we'll change it later. Also, we can fetch it inside ChallengeExpandedCard but it will be slower, require more requests and also needed in homepage so I think it's ok to pass it here for now.

isFirst and isLast are implemented using css

Copy link
Member

@carletex carletex left a comment

Choose a reason for hiding this comment

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

Hey Rinat, thanks for this, great and fast job!

I did a quick first past and left some comments. A general comment: let's aim to add only the stuff that we are sure we need (so we don't end with a bloated project). I prefer to add stuff later on (than removing, which is usually harder)

Do we need logo?

I don't think so. Same as v1.

Will we show every home/portfolio etc buttons on the header or make it as on current sre? (portfolio only)

I think it has some logic depending on your role. For now you can only show "portfolio" when you are logged in an register.

As I understand we don't need balance and network, right?
Hide "view on block explorer" button?
Do we need Eth price at the bottom?

Remove all!

@rin-st
Copy link
Member Author

rin-st commented Feb 21, 2025

Also if you could check the Rainbowkit buttons, they are not readable right now. I'd probably also increase the header buttons a bit, both portfolio and connect/register.

Updated these buttons, thank you! 430403a. Currently buttons (all, not only from header) could be not ideally the same, sometimes they could be a bit different (spacing etc). Probably it's worth to create a button component or find a way to use the same classes everywhere. Possible future PR


main is backmerged now, and "quest" buttons of the first challenge cards are clickable

also, moved enums from schema to separate file, so that enums could be reused in schema and also easy imported from client components because
- it's not possible to get types from schema/db files to client components (but it's not a problem for server components)

  • even if it would be possible, pgEnum's are exported as ts union's, but we don't want to use if (something === "ACCEPTED")
    commit: 256bae2

@rin-st
Copy link
Member Author

rin-st commented Feb 21, 2025

it's not possible to get types from schema/db files to client components (but it's not a problem for server components)

I was wrong about this, types are imported to client components without errors and it's great


updated types so now lots of them are actual, but connected user is still hardcoded c45817f

@rin-st rin-st requested review from Pabl0cks and carletex February 24, 2025 10:18
Copy link
Member

@carletex carletex left a comment

Choose a reason for hiding this comment

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

Good stuff Rinat!! This is looking good.

Added some comments:

  1. Tiny style comments
  2. A hook and service/api refactor (I think it's good if we follow the same approach)
  3. Comments for possible future PRs

Let's merge when 1 & 2 are ready if you want, since this is already big enough.

@rin-st
Copy link
Member Author

rin-st commented Feb 26, 2025

Thanks for review! Added some updates

Let's merge when 1 & 2 are ready if you want, since this is already big enough.

1 is ready but regarding 2 I think it's better to make it in separate PR too since that logic doesn't work yet #23 (comment)

Copy link
Member

@technophile-04 technophile-04 left a comment

Choose a reason for hiding this comment

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

Tysm @rin-st!! This looks great!! I think there is some stuff which would be removed when we do dynamic PR, So let's try to get that in next PR so that we don't loose context and there no dangling files / utils 🙌

@rin-st rin-st mentioned this pull request Feb 27, 2025
Copy link
Member

@carletex carletex left a comment

Choose a reason for hiding this comment

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

I think this is good for a merge. There are a few ToDo + tweaks that we can solve in future PR.

Thanks all!!

@technophile-04 technophile-04 merged commit cbef173 into main Feb 27, 2025
1 check passed
@technophile-04 technophile-04 deleted the homepage-migration branch February 27, 2025 11:49
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.

Homepage migration with the list of challenges + header & footer

4 participants