`Header` component adjustements #357

kpyszkowski · 2024-04-15T18:30:50Z

Goal:

To adjust Header component following latest design changes

Done:

Implement generic Link and NavLink components, a fusion of Chakra's and Router's Link component
Implement Navigation component with animated active item indicator
Added conditional rendering for ConnectWallet component with presence animation

Implemented `Layout` component to account `Header` into router's scope for `NavLink` component support used in `Navigation` component

netlify · 2024-04-15T18:31:07Z

✅ Deploy Preview for acre-dapp-testnet ready!

Name	Link
🔨 Latest commit	`78b0814`
🔍 Latest deploy log	https://app.netlify.com/sites/acre-dapp-testnet/deploys/6620fe0bcc8e6e0008de5d09
😎 Deploy Preview	https://deploy-preview-357--acre-dapp-testnet.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

With proposed approach it's redundant to wrap each page with `Layout` component - it's handled by the wrapper route.

dapp/src/components/Header/Navigation.tsx

dapp/src/components/Header/index.tsx

dapp/src/components/shared/Layout.tsx

dapp/src/components/shared/Link.tsx

r-czajkowski · 2024-04-17T09:10:17Z

dapp/src/components/Header/index.tsx

+// TODO: To be adjusted after project cleanup
+const NAVIGATION_ITEMS: NavigationItemType[] = [
+  { label: "Home", href: routerPath.home },
+  { label: "Activity", href: `${routerPath.activity}/1` },


I think this link should redirect to shared layout for activity so in my opinion we don't need the activity id here (1).

It's only a mock for testing as TODO indicates. The target pages and routing will be completely different after project's pivot.
Updated comment to be more descriptive, ref commit: b02de09

Currently it's still not completely functional. Active state of the link is estimated incorrectly because of hardcoded activity id. I might need to create separate page without the URL segment only for mock.

I didn't do that but tested.
As designs indicates Navigation will only display Season and Dashboard links so dynamic URL segments won't be used, however it's confirmed to be working with parent route created.

The table presents how active state works with dynamic URL segments.

dapp/src/components/Header/ConnectWallet.tsx

kpyszkowski

Addressed comments

ioay · 2024-04-17T17:10:17Z

dapp/src/DApp.tsx

-      <DocsDrawer />
-    </>
-  )
+  return <RouterProvider router={router} />


#358 (comment)

Answered under mentioned comment

ioay · 2024-04-17T17:12:31Z

dapp/src/components/Header/ConnectWallet.tsx

-          leftIcon={<Icon as={BitcoinIcon} boxSize={6} />}
-          onClick={handleConnectBitcoinAccount}
+    <AnimatePresence initial={false}>
+      {(isConnected || !isHomeRoute) && (


Generally, in such cases, we set the condition higher and return null if it does not meet the condition.

It's not true with <AnimatePresence /> component. It requires the render condition to be explicitly defined inside it to capture the rerender.

Ok, so let's create AnimatePresence as a standalone wrapper with children prop and move there this condition.

Not sure why, it's pointless. It will cause the code to shadow. AnimatePresence as a part of Framer Motion has enough semantic meaning. No need to make it a wrapper.

ioay · 2024-04-17T17:22:50Z

dapp/src/components/Header/index.tsx

      <Icon as={AcreLogo} w={20} h={12} />
+      <Navigation items={NAVIGATION_ITEMS} />


Do we want to pass items props instead of importing navigation items directly in the Navigation component?

Why not. It makes Navigation reusable.
It might not be needed in our particular case but who knows.

dapp/src/hooks/router/useIsRoute.ts

kpyszkowski

Addressed comments

ioay · 2024-04-22T08:32:09Z

dapp/src/components/Header/ConnectWallet.tsx

-          leftIcon={<Icon as={BitcoinIcon} boxSize={6} />}
-          onClick={handleConnectBitcoinAccount}
+    <AnimatePresence initial={false}>
+      {(isConnected || !isHomeRoute) && (


Ok, so let's create AnimatePresence as a standalone wrapper with children prop and move there this condition.

ioay · 2024-04-22T08:35:00Z

dapp/src/components/Header/ConnectWallet.tsx

+const containerVariants: Variants = {
+  hidden: { opacity: 0, y: -48 },
+  visible: { opacity: 1, y: 0 },
+}


I believe we can move containerVariants to theme dir.

I'd like to keep animation related variables coupled together. Will not make it.

ioay · 2024-04-22T08:36:17Z

dapp/src/components/Header/Navigation.tsx

+export type NavigationItemType = {
+  label: string
+  href: string
+}


Let's move this type to types/nav.ts/types/navigation.ts

Component related types should be coupled with components. Granularity and separation of concerns is fine unless it's structurally clear. This type definition is not multipurpose nor reusable. It's tightly coupled with the component and it should be held with component or alternatively in separate file close to the component like <ComponentName>.types.ts. Will not make it.

So if it's not multipurpose nor reusable why do we export this type?
In case when we use types in other files we should do it from the types directory that is intended for this purpose. Thanks to this, we prevent creating redundant types in the future, in other places that may want to use such a type. If we use export type we should move it to types.

It's just a props type definition. I don't see any other type definition in types directory.
It's strictly component related. Spreading it across multiple catalogs makes it difficult to find.

In this particular case props can simultaneously act as navigation item type definition so I can eventually approach it another way around and treat it as props type definition.
Ref commit: 2a9b31a

ioay · 2024-04-22T08:38:38Z

dapp/src/components/Header/Navigation.tsx

+              display="block"
+              fontSize="md"
+              lineHeight="md"
+              fontWeight="bold"
+              mb={2}
+              color="grey.500"
+              _activeLink={{ color: "grey.700" }}


Maybe it's worth considering defining these styles in the theme?

Ref commit: bea88fc

ioay · 2024-04-22T08:42:25Z

dapp/src/components/Header/index.tsx

+// TODO: To be adjusted after project pivot/cleanup
+const NAVIGATION_ITEMS: NavigationItemType[] = [
+  { label: "Season 1", href: routerPath.home },
+  { label: "Dashboard", href: routerPath.overview },
+]


Let's create components/Header/Navigation/NavigationItem.ts file and move there this NAVIGATION_ITEMS.

Ref commit: 599a8907

#357 (comment)

ioay · 2024-04-22T08:47:24Z

dapp/src/components/shared/Link.tsx

+export function Link(props: LinkProps) {
+  return <ChakraLink as={RouterLink} {...props} />
+}


Let's separate these two components into two files Link for the Link component and NavLink for NavLink

Ref commit: 4ffdb22

ioay · 2024-04-22T08:54:01Z

dapp/src/components/Header/Navigation.tsx

+          <ListItem key={item.href} pos="relative">
+            <NavLink


Let's move this logic to components/Header/Navigation/NavigationItem.ts file.

Ref commit: cc68712

Ref commit: 599a8907

Don't see any new commits on the list(https://github.com/thesis/acre/pull/357/commits), did you push them? :)

kkosiorowska

Switching navigation looks great 🚀 I have left just a small comment, which can be improved later if needed.

kkosiorowska · 2024-04-30T09:41:13Z

dapp/src/components/Header/Navigation/index.ts

+export { default as Navigation } from "./Navigation"
+export { default as NavigationItem } from "./NavigationItem"


Some time ago we discussed the names and directory structure for components. Some of them are described here.

I believe that it would be good for us to have one approach together and keep a kind of order. So I'm wondering if the index.tsx file should not export the main component. The name of the directory indicates this and this component uses NavigationItem inside. That's why I'm thinking about it. The above solution puts these components a bit as sibling components. I'm curious about your opinion.

Actually that's a good catch to remove NavigationItem export. NavigationItem shouldn't be used as a standalone part of the UI. It's just a child of Navigation and with that said it should be used only in parent component. That's why I removed this export, ref commit: 689154b

About the index files - generally speaking it hurts me a little to be honest 🤷🏻‍♂️
Because I have a huge problem to keep the source code in index files.
As the name suggests, they are used for indexing - so to organise exports, namespacing.

And you can get stuck with several files open at the same time, all with the same name (index.ts). It is then difficult to maintain it and traverse through the code.

What I have proposed is, in my opinion, the ideal spot, because:

we have one component per file,

the file name reflects the content

there is namespacing which means everything is oriented around the Navigation by including it in a common directory

imports are consistent and we get rid of unreadable "../Navigation/Navigation" paths

About the module/default export it's a topic to discuss. I don't have a preference here, maybe slightly leaning towards default exports.
However my definitive conclusion is to stop writing code in index files.
Index file is for indexing - full stop 😆

Closes: #363 ## Goal: To implement Landing Page This PR aggregates all component PRs to finally complete the implementation of landing page ## Component PRs: The list of PRs is given in the issue's description. Each component PR merges to this PR. - #357 - #358 - #364 - #368 - #369 ## Designs: <img width="1205" alt="image" src="https://github.com/thesis/acre/assets/11503879/be6b6ed2-a808-483e-9daf-43d2c042cba4">

Implement Navigation component

5d8343a

Implemented `Layout` component to account `Header` into router's scope for `NavLink` component support used in `Navigation` component

kpyszkowski marked this pull request as draft April 15, 2024 18:31

kpyszkowski changed the title ~~Draft: Header component adjustements~~ Header component adjustements Apr 15, 2024

kpyszkowski mentioned this pull request Apr 15, 2024

dApp Landing Page MVP #355

Closed

kpyszkowski self-assigned this Apr 15, 2024

Define Layout as wrapper route

06596ba

With proposed approach it's redundant to wrap each page with `Layout` component - it's handled by the wrapper route.

kpyszkowski marked this pull request as ready for review April 15, 2024 23:45

kpyszkowski requested a review from ioay April 15, 2024 23:45

kpyszkowski added 3 commits April 16, 2024 01:50

Implement conditional ConnectWallet rendering

c0b0df3

Remove Ethereum wallet connection button

0c707f7

Implement ConnectWallet's presence animation

a06c1de

kpyszkowski force-pushed the header-navigation branch from 77a7bb2 to a06c1de Compare April 15, 2024 23:51

Adjust Header's padding

e54282f

r-czajkowski reviewed Apr 17, 2024

View reviewed changes

kpyszkowski added 6 commits April 17, 2024 12:52

Refactor Navigation component

5cedbf2

Replace color values with semantic tokens

dec2bcc

Wrap Navigation component with nav element

bc41b76

Move Header outside main element

5d25c89

Update comment to be more descriptive

b02de09

Remove explicit null value in ConnectWallet

7ce1664

kpyszkowski commented Apr 17, 2024

View reviewed changes

kpyszkowski requested a review from r-czajkowski April 17, 2024 11:10

r-czajkowski previously approved these changes Apr 17, 2024

View reviewed changes

ioay reviewed Apr 17, 2024

View reviewed changes

Rename hook

78b0814

kpyszkowski dismissed r-czajkowski’s stale review via 78b0814 April 18, 2024 11:03

kpyszkowski commented Apr 18, 2024

View reviewed changes

kpyszkowski changed the base branch from main to landing-page April 18, 2024 11:43

Merge branch 'landing-page' into header-navigation

5ff78eb

kpyszkowski and others added 2 commits April 18, 2024 13:53

Adjust navigation items

66fc680

Merge branch 'landing-page' into header-navigation

0548299

kpyszkowski marked this pull request as draft April 18, 2024 12:57

kpyszkowski marked this pull request as ready for review April 18, 2024 12:57

ioay reviewed Apr 22, 2024

View reviewed changes

kpyszkowski and others added 7 commits April 22, 2024 18:12

Encaplsulate NavigationItem component

cc68712

Define 'navigation' Link theme variant

bea88fc

Encapsulate NavLink component into separate file

4ffdb22

Move navigation item type definition to types dir

2a9b31a

Merge branch 'landing-page' into header-navigation

856e70a

Merge branch 'landing-page' into header-navigation

5393743

Resolve merge errors

d286b0f

kpyszkowski requested review from ioay, kkosiorowska and r-czajkowski and removed request for ioay April 25, 2024 10:08

kkosiorowska approved these changes Apr 30, 2024

View reviewed changes

Remove NavigationItem export

689154b

kkosiorowska merged commit e1b8d0a into landing-page Apr 30, 2024
20 checks passed

kkosiorowska deleted the header-navigation branch April 30, 2024 14:29

nkuba mentioned this pull request May 3, 2024

Implement Landing Page #370

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`Header` component adjustements #357

`Header` component adjustements #357

kpyszkowski commented Apr 15, 2024 •

edited

Loading

netlify bot commented Apr 15, 2024 •

edited

Loading

r-czajkowski Apr 17, 2024

kpyszkowski Apr 17, 2024

kpyszkowski Apr 17, 2024 •

edited

Loading

kpyszkowski left a comment

ioay Apr 17, 2024

kpyszkowski Apr 18, 2024

ioay Apr 17, 2024

kpyszkowski Apr 18, 2024

ioay Apr 22, 2024

kpyszkowski Apr 22, 2024

ioay Apr 17, 2024

kpyszkowski Apr 18, 2024

kpyszkowski left a comment

ioay Apr 22, 2024

ioay Apr 22, 2024

kpyszkowski Apr 22, 2024

ioay Apr 22, 2024 •

edited

Loading

kpyszkowski Apr 22, 2024

ioay Apr 22, 2024

kpyszkowski Apr 22, 2024

ioay Apr 22, 2024

kpyszkowski Apr 22, 2024 •

edited

Loading

ioay Apr 22, 2024 •

edited

Loading

kpyszkowski Apr 22, 2024

ioay Apr 22, 2024

ioay Apr 22, 2024

kpyszkowski Apr 22, 2024 •

edited

Loading

ioay Apr 22, 2024

kpyszkowski Apr 22, 2024 •

edited

Loading

ioay Apr 22, 2024

kkosiorowska left a comment

kkosiorowska Apr 30, 2024

kpyszkowski Apr 30, 2024 •

edited

Loading

		<Icon as={AcreLogo} w={20} h={12} />
		<Navigation items={NAVIGATION_ITEMS} />

		export { default as Navigation } from "./Navigation"
		export { default as NavigationItem } from "./NavigationItem"

Header component adjustements #357

Header component adjustements #357

Conversation

kpyszkowski commented Apr 15, 2024 • edited Loading

Goal:

Done:

netlify bot commented Apr 15, 2024 • edited Loading

✅ Deploy Preview for acre-dapp-testnet ready!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kpyszkowski Apr 17, 2024 • edited Loading

Choose a reason for hiding this comment

kpyszkowski left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kpyszkowski left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ioay Apr 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kpyszkowski Apr 22, 2024 • edited Loading

Choose a reason for hiding this comment

ioay Apr 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kpyszkowski Apr 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kpyszkowski Apr 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kkosiorowska left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kpyszkowski Apr 30, 2024 • edited Loading

Choose a reason for hiding this comment

`Header` component adjustements #357

`Header` component adjustements #357

kpyszkowski commented Apr 15, 2024 •

edited

Loading

netlify bot commented Apr 15, 2024 •

edited

Loading

kpyszkowski Apr 17, 2024 •

edited

Loading

ioay Apr 22, 2024 •

edited

Loading

kpyszkowski Apr 22, 2024 •

edited

Loading

ioay Apr 22, 2024 •

edited

Loading

kpyszkowski Apr 22, 2024 •

edited

Loading

kpyszkowski Apr 22, 2024 •

edited

Loading

kpyszkowski Apr 30, 2024 •

edited

Loading