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

Enable Consensus routing #1129

Merged
merged 1 commit into from
Jan 18, 2024
Merged

Enable Consensus routing #1129

merged 1 commit into from
Jan 18, 2024

Conversation

buberdds
Copy link
Contributor

@buberdds buberdds commented Jan 4, 2024

Consensus mockups don't match most of our ParaTimes views so we prob need to setup separated views. To discuss how we want to handle routes

here we can keep dynamic segments for ParaTimes and we will be able to add specific Consensus (static segment) routes

          {
            path: 'consensus',
            errorElement: withDefaultTheme(<RoutingErrorPage />),
            loader: scopeLoader,
            children: [
              {
                path: '',
                element: <ConsensusDashboardPage />,
              },
            ],
          },
          // our current runtimes paths

Will match backend design (consensus - runtimes separation) more. We keep current Paratimes as is. W can rename some stuff like :layer to :runtime if we go with splats.

possible routing options:

  • dynamic segments for ParaTimes static for Consensus
  • dynamic segments only
  • static segments
  • (breaking change) re-define routes hierarchy (mainnet/runtime/:layer, mainnet/consensus)

Copy link

github-actions bot commented Jan 4, 2024

Deployed to Cloudflare Pages

Latest commit: 855784529a64d320997f82ab895d65aab1c2e006
Status:✅ Deploy successful!
Preview URL: https://48141217.oasis-explorer.pages.dev

@buberdds buberdds force-pushed the mz/consensusRoutes branch 3 times, most recently from a6b5e54 to de05a9b Compare January 15, 2024 15:51
@buberdds buberdds marked this pull request as ready for review January 15, 2024 15:55
src/routes.tsx Outdated Show resolved Hide resolved
@buberdds buberdds changed the title Switch to splat routes to enable Consensus routes in the future Enable Consensus routing Jan 17, 2024
@buberdds buberdds requested a review from lukaw3d January 17, 2024 15:25
Comment on lines +18 to +19
layer: Layer | undefined
network: Network | undefined
Copy link
Member

Choose a reason for hiding this comment

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

I don't think these fields can be undefined. It would be caught by loaders. I think useScopeParam only needs to check that one of the useRouteLoaderData is found

Copy link
Member

Choose a reason for hiding this comment

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

But that was also true previously, so was .valid not needed, and always true?

Copy link
Member

Choose a reason for hiding this comment

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

or do we never use the errors thrown in loaders

Copy link
Member

Choose a reason for hiding this comment

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

large refactor in #1159

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Refactor looks cool, thanks! let's go with #1159 in this case and just sync changelog number there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah I can see the plan is to merge this one first.

@buberdds buberdds closed this Jan 18, 2024
@buberdds buberdds reopened this Jan 18, 2024
@buberdds buberdds merged commit 93974e4 into master Jan 18, 2024
14 checks passed
@buberdds buberdds deleted the mz/consensusRoutes branch January 18, 2024 11:23
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.

2 participants