feat: Improve tabs on mobile#897
Conversation
|
@rogaldh is attempting to deploy a commit to the Solana Foundation Team on Vercel. A member of the Team first needs to authorize it. |
Greptile SummaryThis PR replaces the legacy Bootstrap tab list with a new Key changes:
Confidence Score: 4/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant Layout as AddressLayout / BlockLayout
participant NT as NavigationTabs (client)
participant BNT as BaseNavigationTabs
participant Ctx as NavigationTabsContext
participant NTL as NavigationTabLink (async child)
participant Router as Next.js Router
Layout->>NT: tabs={staticTabs} buildHref={fn}
NT->>NT: activeValue = useSelectedLayoutSegment() ?? ''
NT->>BNT: tabs, activeValue, onSelectChange, buildHref
BNT->>Ctx: provide {activeValue, buildHref, registerTab, staticPaths, unregisterTab}
Note over BNT: Renders <select> (mobile) + tablist div (desktop)
NTL->>Ctx: useNavigationTabsContext()
NTL->>Ctx: registerTab({path, title}) via useEffect
Ctx-->>BNT: registeredTabs state updated
BNT->>BNT: allTabs = [...staticTabs, ...registeredTabs.filter(not in staticPaths)]
BNT-->>BNT: <select> re-renders with allTabs options
Note over BNT: Desktop: staticTabs.map(TabLink) + {children} (NavigationTabLinks)
Note over BNT: Mobile: allTabs.map(option) in <select>
Router->>NT: user selects option (mobile)
NT->>Router: router.push(buildHref(path), {scroll: false})
Reviews (5): Last reviewed commit: "resolve comments" | Re-trigger Greptile |
|
@greptile-apps check again |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
@greptile-apps check again |
|
@greptile-apps check again |
| import React from 'react'; | ||
|
|
||
| import { cn } from '@/app/components/shared/utils'; | ||
| import { useNavigationTabsContext } from '@/app/entities/navigation-tabs/model/navigation-tabs-context'; |
There was a problem hiding this comment.
suggestion: While there’s nothing critically wrong here, this does not align with classic FSD principles. According to FSD, entities should represent a domain concept (e.g. address, transaction, etc.) rather than merely grouping UI atoms. This should be categorized under shared, per the methodology. Additionally, we should never expose module internals (except for shared). If there’s concern about making the barrel too large, it may indicate that the entity abstraction should be reevaluated.
| React.useEffect(() => { | ||
| ctx.registerTab({ path, title }); | ||
| return () => ctx.unregisterTab(path); | ||
| // registerTab/unregisterTab are stable (useCallback with []), ctx excluded intentionally |
There was a problem hiding this comment.
question: Assuming these are stable, could we destructure registerTab and unregisterTab and create a proper dependency list?
| activeValue: 'rewards', | ||
| tabs: BLOCK_TABS, | ||
| }, | ||
| parameters: { |
There was a problem hiding this comment.
issue: Doesn't work, should be:
globals: {
viewport: { value: 'mobile1', isRotated: false },
},| e-bg-heavy-metal-900 e-px-4 e-py-2.5 e-text-sm | ||
| e-text-neutral-200 e-outline-none; | ||
| margin-left: -12px; | ||
| margin-right: -12px; |
There was a problem hiding this comment.
suggestion: Perhaps these hacks should have a comment explaining what they depend on
| const tabComponents = getTabs(pubkey, account).concat(getCustomLinkedTabs(pubkey, account)); | ||
| const navigationTabs = getNavigationTabs(pubkey, account); | ||
|
|
||
| if (tab && tabComponents.filter(tabComponent => tabComponent.tab.slug === tab).length === 0) { |
There was a problem hiding this comment.
This guard is gone, please rollback those changes
remove non existent tab fix: restructure the navigation to make it follow fsd
Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
a8e8d13 to
9e3d12c
Compare
|
@greptile-apps check again |
Description
Improve tabs to make them work better on mobile by replacing plain tabs with
<select>Type of change
Screenshots
Desktop:
Mobile:
Testing
All tests work.
storiesare provided that perform checks for the proper number of tabs according to the tab nature (static/dynamic)Related Issues
HOO-107
hoodieshq#50 hoodieshq#49 hoodieshq#48 hoodieshq#46 hoodieshq#57
Checklist
build:infoscript to update build information