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

feat: decouple routes and navbar #9051

Draft
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

JoeChenJ
Copy link
Contributor

Description

Decouple routes and navbar.

Top-level routes are now able to be declared without having to add fluff related to navbar.

Issue (if applicable)

closes #9023

Risk

High Risk PRs Require 2 approvals

Low

What protocols, transaction types, wallets or contract interactions might be affected by this PR?

Testing

Engineering

Operations

  • 🏁 My feature is behind a flag and doesn't require operations testing (yet)

Screenshots (if applicable)

@JoeChenJ JoeChenJ requested a review from a team as a code owner March 17, 2025 05:27
: !route.disable && !route.hide && !route.mobileNav,
.filter(route => 'label' in route)
.filter((route): route is NavRoute =>
isLargerThanMd ? !route.disable && !route.hideDesktop : !route.disable && !route.mobileNav,
Copy link
Contributor

Choose a reason for hiding this comment

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

This won't work with the new types and would require additional narrowing e.g type guards of sorts.

@gomesalexandre
Copy link
Contributor

gm @JoeChenJ! Thanks for the contribution.
Unfortunately this is failing CI:
image

I've added some commentst your PR for posteriority and moved to draft rather than closing to keep it clear work had been started from an external contributor, but will eventually close this as we work on this one. I'll cherry-pick your commit as a starting point when we do so.

This is an issue we would only want CODEOWNERS to take on, as there is much more to this than just type changes and a whole routing refactor is required, with large risks. Sorry about this! Please consider this closed.

@gomesalexandre gomesalexandre marked this pull request as draft March 17, 2025 07:55
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.

Decouple routes and navbar
2 participants