Changes assets -> home, updates styles, adds kernel links #24
Conversation
There was a problem hiding this comment.
I've left some comments.
The main question for us to ponder are
- where is the boundary between the open gift of the non-custodial wallet vs. being a part of the Kernel community as a member
- how should we handle the transition phase of enforcing this boundary before application and review apps are in effect
packages/wallet/src/views/Home.jsx
Outdated
| } | ||
| ] | ||
|
|
||
| const kernelCards = [ |
There was a problem hiding this comment.
We should keep in mind this is not specific to the Kernel. The concepts we use is different roles represented by a single number visualized by concentric circles.
What you are trying to express in this model is showing different links based on a user's role, but there is no concept of the Kernel in the code base.
The intended way to use this is there's some threshold where you graduate to an outer circle represented as a decrease in the number that is your role.
We are still missing the application and review apps which will be the primary way to move members from the permissionless, open Web3 role to a member role.
Therefore, this should be renamed as memberCards.
packages/wallet/src/views/Home.jsx
Outdated
| { | ||
| title: 'UnProfile', | ||
| description: 'Unprofile yourself', | ||
| url: 'https://staging.unprofile.kernel.community/', |
There was a problem hiding this comment.
This url is going to cause issues when deploying to a different environment other than staging.
There is an environment variable REACT_APP_DEPLOY_TARGET that will tell you what the target environment is.
We should make the URLs dependent on the target.
There's an open issue to update the footer to include links to the apps and the code should be shared across both use cases. #19
There was a problem hiding this comment.
@andytudhope you may beat Alon to this, so if you want to use the guidance in #19 ("How to get the correct domains") to create a helper that returns the correct url for each app, go ahead. just let him know so that he can reuse it for the footer.
There was a problem hiding this comment.
A couple of comments, plus a style nit: can you try to shift the cards to the right such that the "Welcome" text looks centered in between the left/right bounds of the cards? (see markings in the screenshot) This could be accomplished by shifting all of the cards together, or just the "For Everyone" cards.
packages/wallet/src/App.js
Outdated
| <Route path='/assets' element={<Navigate to='/assets/overview' replace />} /> | ||
| <Route path='/assets/overview' element={<Assets />} /> | ||
| <Route path='/home' element={<Navigate to='/home/overview' replace />} /> | ||
| <Route path='/home/overview' element={<Home />} /> |
There was a problem hiding this comment.
is there any problem with just having the one /home route and deleting /home/overview?
There was a problem hiding this comment.
I think we'll want more pages under home so I feel comfortable leaving this here.
packages/wallet/src/views/Home.jsx
Outdated
| { | ||
| title: 'UnProfile', | ||
| description: 'Unprofile yourself', | ||
| url: 'https://staging.unprofile.kernel.community/', |
There was a problem hiding this comment.
@andytudhope you may beat Alon to this, so if you want to use the guidance in #19 ("How to get the correct domains") to create a helper that returns the correct url for each app, go ahead. just let him know so that he can reuse it for the footer.
packages/wallet/src/views/Home.jsx
Outdated
| </div> | ||
| </a> | ||
| ) | ||
| })} |
There was a problem hiding this comment.
Since this is now being duplicated, you can break out this returned component into a named component defined at the top level, like
const LinkCard = (cardConfig, index) => {
...
}
…nity#24) * Adventures: introduce Page component with proper navbar * PR fixes
| }) | ||
| return Card | ||
| } | ||
|
|
There was a problem hiding this comment.
- It's helpful to name something in the plural when it refers to a collection of things, so maybe
LinksCardorLinkCardsdepending on how you're thinking about it. const Cardis unnecessary; you can justreturn cardConfig.map((...and remove line 121.
Begins to attend to #16 by changing the landing page to
Homeand adding Kernel-specific links to our growing family of focused web apps.