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

Add support for handling child tab routes #1049

Merged
merged 1 commit into from Nov 29, 2023
Merged

Conversation

buberdds
Copy link
Contributor

@buberdds buberdds commented Nov 29, 2023

Alternate solution to #910

Keep router tab selected for a direct child route. Needed for NFT project.

Copy link

github-actions bot commented Nov 29, 2023

Deployed to Cloudflare Pages

Latest commit: 6f47aa1928ee9459157dcf35fed6216b894d4c5f
Status:✅ Deploy successful!
Preview URL: https://e8122e01.oasis-explorer.pages.dev

@buberdds buberdds changed the title Add support for hierarchical routes Add support for hierarchical tab routes Nov 29, 2023

if (!targetTab) {
/// the last index is the current route, -2 is the first parent in route hierarchy
const parentPathname = matches?.at(-2)?.pathname
Copy link
Collaborator

Choose a reason for hiding this comment

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

Haven't run this, but hardcoding this to 2 level deep seems like a wrong approach. Can we take the first child of matches as the top parent? To support "true" hierarchy?

Copy link
Contributor Author

@buberdds buberdds Nov 29, 2023

Choose a reason for hiding this comment

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

oh yeah I just need to handle direct child routes - I've updated commit messages to be more specific. "The first child of matches" is root path '/' I am not sure how it can help. To handle any number of nested child routes we would need to do sth like: based on a current route, find an index in matches based on the closest to value in tabs array.

@@ -18,13 +18,21 @@ function getPathname(tab: { to: string }) {

export function RouterTabs<Context>({ tabs, context }: RouterTabsProps<Context>) {
const { pathname } = useLocation()
const currentTab = tabs.find(tab => getPathname(tab) === pathname)
let targetTab = tabs.find(tab => getPathname(tab) === pathname)
const matches = useMatches()
Copy link
Member

Choose a reason for hiding this comment

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

Ah, nice! If only there was also "useMatchAtCurrentOutletLevel"

src/app/components/RouterTabs/index.tsx Outdated Show resolved Hide resolved
Comment on lines 24 to 28
if (!targetTab) {
/// the last index is the current route, -2 is the first parent in route hierarchy
const parentPathname = matches?.at(-2)?.pathname
// route match does not include hash, so we need to remove it from comparison
targetTab = tabs.find(tab => tab.to.split('#')[0] === parentPathname)
}
Copy link
Member

Choose a reason for hiding this comment

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

(Difference between #910: this makes matchPartialPath? the default instead of optional. Makes sense. I think matching subpaths is expected behavior everywhere)

@@ -18,13 +18,21 @@ function getPathname(tab: { to: string }) {

export function RouterTabs<Context>({ tabs, context }: RouterTabsProps<Context>) {
Copy link
Member

Choose a reason for hiding this comment

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

Ideally add some tests, e.g. what tab is selected if tabs are

tabs = [
  "/mainnet/sapphire/address/oasis1qry5d5zxs58w9deu5ara5zaamhkzqtg99s00ls90",
  "/mainnet/sapphire/address/oasis1qry5d5zxs58w9deu5ara5zaamhkzqtg99s00ls90/token-transfers#transfers",
  "/mainnet/sapphire/address/oasis1qry5d5zxs58w9deu5ara5zaamhkzqtg99s00ls90/tokens/erc-20#tokens",
  "/mainnet/sapphire/address/oasis1qry5d5zxs58w9deu5ara5zaamhkzqtg99s00ls90/tokens/erc-721#tokens",
  "/mainnet/sapphire/address/oasis1qry5d5zxs58w9deu5ara5zaamhkzqtg99s00ls90/code#code",
]

if location is:
/mainnet/sapphire/address/oasis1qry5d5zxs58w9deu5ara5zaamhkzqtg99s00ls90/tokens/erc-721
/mainnet/sapphire/address/oasis1qry5d5zxs58w9deu5ara5zaamhkzqtg99s00ls90/tokens/erc-721/token1#token1
/mainnet/sapphire/address/oasis1qry5d5zxs58w9deu5ara5zaamhkzqtg99s00ls90/other
/mainnet/sapphire/address/oasis1qry5d5zxs58w9deu5ara5zaamhkzqtg99s00ls90#transfers

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will add them once I will wrap up other NFT UI work.

@buberdds buberdds force-pushed the mz/hierarchical-tabs branch 2 times, most recently from fd8cdd3 to 21e871f Compare November 29, 2023 21:13
@buberdds buberdds changed the title Add support for hierarchical tab routes Add support for handling child tab routes Nov 29, 2023
@buberdds buberdds merged commit 05b0445 into master Nov 29, 2023
8 checks passed
@buberdds buberdds deleted the mz/hierarchical-tabs branch November 29, 2023 21:31
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.

None yet

3 participants