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

Implement router transitions #358

Merged
merged 6 commits into from
Apr 23, 2024
Merged

Implement router transitions #358

merged 6 commits into from
Apr 23, 2024

Conversation

kpyszkowski
Copy link
Contributor

Ref: #355

Goal:

To animate the content rendered by React Router - now it slides in and out in y-axis as the route changes.

Pros:

  • better user experience
  • looks better ofc 💅🏻

Misc:

Refactored router creation strategy to use components instead of factory function.

Now content slides in and out in y-axis as the route changes.
Also refactored router creation strategy to use components instead of
factory function.
Copy link

netlify bot commented Apr 15, 2024

Deploy Preview for acre-dapp-testnet ready!

Name Link
🔨 Latest commit 679876d
🔍 Latest deploy log https://app.netlify.com/sites/acre-dapp-testnet/deploys/661eeab4862a6e0008adf1d1
😎 Deploy Preview https://deploy-preview-358--acre-dapp-testnet.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

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

@kpyszkowski kpyszkowski self-assigned this Apr 15, 2024
Comment on lines 8 to 22
export function Router() {
return (
<BrowserRouter>
<Routes>
<Route path="/" element={<Layout />}>
<Route index path={routerPath.home} element={<OverviewPage />} />
<Route
path={`${routerPath.activity}/:activityId`}
element={<ActivityPage />}
/>
</Route>
</Routes>
</BrowserRouter>
)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Why it was changed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

With <Layout /> parent route nested routes in JSX are easier to read and maintain instead of object notation (array of children) elements when using createBrowserRouter factory function.

dapp/src/components/shared/Layout.tsx Show resolved Hide resolved
<DocsDrawer />
</>
)
return <Router />
Copy link
Contributor

Choose a reason for hiding this comment

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

In case when DApp returns only the Router component maybe we should consider moving this logic to the parent and then deleting this file simplifying the structure. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In this case I'd opt for slightly different solution - to leave routing separated as is - routing is kind of hermetic area with it's "natural" namespace and it's good to have it separated in case the application scales and the need of new routes might occur.
My suggestion is to remove the useInitApp hook. Now when DApp component has such a simple structure this hook is only a redundant wrapper to click through when inspecting the code.

What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd rather leave useInitApp as it is. I'm considering whether DAppProviders should return GlobalStyles(it's not a provider), so maybe we should move it to DApp component to be semantically correct.

Copy link
Contributor Author

@kpyszkowski kpyszkowski Apr 22, 2024

Choose a reason for hiding this comment

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

Ref commit: eb37fa9

Copy link
Contributor Author

@kpyszkowski kpyszkowski left a comment

Choose a reason for hiding this comment

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

Addressed comments

@kpyszkowski kpyszkowski changed the base branch from main to landing-page April 18, 2024 11:55
Copy link
Contributor

@ioay ioay left a comment

Choose a reason for hiding this comment

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

LGTM 🚀 just one minor comment

@ioay ioay merged commit 029d9f0 into landing-page Apr 23, 2024
20 checks passed
@ioay ioay deleted the router-transitions branch April 23, 2024 09:42
@nkuba nkuba mentioned this pull request May 3, 2024
kkosiorowska added a commit that referenced this pull request May 9, 2024
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">
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.

3 participants