-
Notifications
You must be signed in to change notification settings - Fork 18
Homepage migration #23
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
Conversation
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 Also in normal 14inch it looks like this, there is empty right space |
packages/nextjs/app/page.tsx
Outdated
{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} | ||
/> | ||
))} |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this 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!
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
also, moved enums from schema to separate file, so that enums could be reused in schema and also easy imported from client components because
|
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 |
There was a problem hiding this 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:
- Tiny style comments
- A hook and service/api refactor (I think it's good if we follow the same approach)
- Comments for possible future PRs
Let's merge when 1 & 2 are ready if you want, since this is already big enough.
Thanks for review! Added some updates
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) |
There was a problem hiding this 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 🙌
There was a problem hiding this 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!!
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:
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?
Note: I can't add more than one reviewer
cc @technophile-04 @Pabl0cks
Fixes #8