-
Notifications
You must be signed in to change notification settings - Fork 0
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
chore: style navigation tabs #57
Changes from all commits
46c8446
36d5b15
35ddc9c
dbda710
04ca16e
f7af168
df44dd2
a9e9aa0
e330b43
a8a5b22
bcdb942
1fe3d9e
68636fa
e6c7b71
c67a298
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -86,6 +86,7 @@ const theme = createTheme({ | |
}, | ||
}, | ||
}, | ||
spacing: 1, | ||
}) | ||
|
||
export { theme } |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,91 @@ | ||
import InfoOutlinedIcon from '@mui/icons-material/InfoOutlined' | ||
import PostAddIcon from '@mui/icons-material/PostAdd' | ||
import SettingsIcon from '@mui/icons-material/Settings' | ||
import Tab from '@mui/material/Tab' | ||
import MuiTabs from '@mui/material/Tabs' | ||
import { styled } from '@mui/material/styles' | ||
import { useLocation, useNavigate } from '@tanstack/react-router' | ||
import { defineMessages, useIntl } from 'react-intl' | ||
|
||
import type { FileRoutesById } from '../routeTree.gen' | ||
import { Text } from './Text' | ||
|
||
const m = defineMessages({ | ||
setting: { | ||
id: 'tabBar.label.settings', | ||
defaultMessage: 'Settings', | ||
}, | ||
about: { | ||
id: 'tabBar.label.about', | ||
defaultMessage: 'About', | ||
}, | ||
}) | ||
|
||
const MapTabStyled = styled(MapTab)({ | ||
width: 60, | ||
'& MuiButtonBase.Mui-selected': { color: '#000' }, | ||
}) | ||
|
||
export const Tabs = () => { | ||
const navigate = useNavigate() | ||
const { formatMessage } = useIntl() | ||
const location = useLocation() | ||
return ( | ||
<MuiTabs | ||
sx={{ | ||
pb: 20, | ||
pt: 20, | ||
'& .MuiTabs-flexContainer': { | ||
height: '100%', | ||
}, | ||
}} | ||
onChange={(_, value) => navigate({ to: value as MapTabRoute })} | ||
orientation="vertical" | ||
value={location.pathname} | ||
TabIndicatorProps={{ style: { backgroundColor: 'transparent' } }} | ||
> | ||
<MapTabStyled | ||
data-testid="tab-observation" | ||
icon={<PostAddIcon />} | ||
value={'/tab1'} | ||
/> | ||
{/* This is needed to properly space the items. Originally used a div, but was causing console errors as the Parent component passes it props, which were invalid for non-tab components */} | ||
<Tab disabled={true} sx={{ flex: 1 }} /> | ||
|
||
<MapTabStyled | ||
icon={<SettingsIcon />} | ||
label={ | ||
<Text style={{ fontSize: 10 }} kind="title"> | ||
{formatMessage(m.setting)} | ||
</Text> | ||
} | ||
value={'/tab2'} | ||
/> | ||
<MapTabStyled | ||
icon={<InfoOutlinedIcon />} | ||
label={ | ||
<Text style={{ fontSize: 10 }} kind="title"> | ||
{formatMessage(m.about)} | ||
</Text> | ||
} | ||
value={'/tab2'} | ||
/> | ||
</MuiTabs> | ||
) | ||
} | ||
|
||
type TabProps = React.ComponentProps<typeof Tab> | ||
|
||
type MapTabRoute = { | ||
[K in keyof FileRoutesById]: K extends `${'/(MapTabs)/_Map'}${infer Rest}` | ||
? Rest extends '' | ||
? never | ||
: `${Rest}` | ||
: never | ||
}[keyof FileRoutesById] | ||
|
||
type MapTabProps = Omit<TabProps, 'value'> & { value: MapTabRoute } | ||
|
||
function MapTab(props: MapTabProps) { | ||
return <Tab {...props} /> | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,20 +1,54 @@ | ||
import type { ReactNode } from 'react' | ||
import { | ||
RouterProvider, | ||
createRootRoute, | ||
createRoute, | ||
createRouter, | ||
} from '@tanstack/react-router' | ||
import { render, screen } from '@testing-library/react' | ||
import { expect, test, vi } from 'vitest' | ||
import { expect, test } from 'vitest' | ||
|
||
import { IntlProvider } from '../../contexts/IntlContext' | ||
import { MapLayout } from './_Map' | ||
|
||
vi.mock('@tanstack/react-router', () => ({ | ||
useNavigate: vi.fn(() => { | ||
return { navigate: vi.fn() } | ||
}), | ||
createFileRoute: vi.fn(() => { | ||
// eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
return (options: any) => ({ component: options.component }) // Mocked implementation | ||
}), | ||
Outlet: () => <div>Mocked Outlet</div>, | ||
})) | ||
|
||
test('renders something in the jsdom', () => { | ||
render(<MapLayout />) | ||
expect(screen).toBeDefined() | ||
const rootRoute = createRootRoute({}) | ||
|
||
const Wrapper = ({ children }: { children: ReactNode }) => ( | ||
<IntlProvider>{children}</IntlProvider> | ||
) | ||
|
||
// Creates a stubbed out router. We are just testing whether the navigation gets passed the correct route (aka "/tab1" or "/tab2") so we do not need the actual router and can just intecept the navgiation state. | ||
const mapRoute = createRoute({ | ||
getParentRoute: () => rootRoute, | ||
id: 'map', | ||
component: MapLayout, | ||
}) | ||
|
||
const catchAllRoute = createRoute({ | ||
getParentRoute: () => mapRoute, | ||
path: '$', | ||
component: () => null, | ||
}) | ||
|
||
const routeTree = rootRoute.addChildren([mapRoute.addChildren([catchAllRoute])]) | ||
|
||
const router = createRouter({ routeTree }) | ||
|
||
test('clicking tabs navigate to correct tab', () => { | ||
// @ts-expect-error - typings | ||
render(<RouterProvider router={router} />, { wrapper: Wrapper }) | ||
const settingsButton = screen.getByText('Settings') | ||
settingsButton.click() | ||
const settingsRouteName = router.state.location.pathname | ||
expect(settingsRouteName).toStrictEqual('/tab2') | ||
|
||
const observationTab = screen.getByTestId('tab-observation') | ||
observationTab.click() | ||
const observationTabRouteName = router.state.location.pathname | ||
expect(observationTabRouteName).toStrictEqual('/tab1') | ||
|
||
const aboutTab = screen.getByText('About') | ||
aboutTab.click() | ||
const aboutTabRoute = router.state.location.pathname | ||
expect(aboutTabRoute).toStrictEqual('/tab2') | ||
}) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,49 +1,42 @@ | ||
import * as React from 'react' | ||
import { Paper } from '@mui/material' | ||
import Tab from '@mui/material/Tab' | ||
import Tabs from '@mui/material/Tabs' | ||
import { Outlet, createFileRoute, useNavigate } from '@tanstack/react-router' | ||
import { Suspense } from 'react' | ||
import { CircularProgress, Paper } from '@mui/material' | ||
cimigree marked this conversation as resolved.
Show resolved
Hide resolved
|
||
import { styled } from '@mui/material/styles' | ||
import { Outlet, createFileRoute } from '@tanstack/react-router' | ||
|
||
import type { FileRoutesById } from '../../routeTree.gen' | ||
import { VERY_LIGHT_GREY, WHITE } from '../../colors' | ||
import { Tabs } from '../../components/Tabs' | ||
|
||
const Container = styled('div')({ | ||
display: 'flex', | ||
backgroundColor: WHITE, | ||
height: '100%', | ||
}) | ||
|
||
export const Route = createFileRoute('/(MapTabs)/_Map')({ | ||
component: MapLayout, | ||
}) | ||
|
||
export function MapLayout() { | ||
const navigate = useNavigate() | ||
const renderCount = React.useRef(0) | ||
renderCount.current = renderCount.current + 1 | ||
return ( | ||
<div> | ||
<Tabs | ||
onChange={(_, value) => navigate({ to: value as MapTabRoute })} | ||
orientation="vertical" | ||
> | ||
<MapTab label="Tab 1" value={'/tab1'} /> | ||
<MapTab label="Tab 2" value={'/tab2'} /> | ||
</Tabs> | ||
<Paper> | ||
<Outlet /> | ||
<Container> | ||
<Paper elevation={3} sx={{ display: 'flex' }}> | ||
<Tabs /> | ||
<div | ||
style={{ | ||
width: 300, | ||
borderLeftColor: VERY_LIGHT_GREY, | ||
borderLeftWidth: '1px', | ||
borderLeftStyle: 'solid', | ||
}} | ||
> | ||
<Suspense fallback={<CircularProgress />}> | ||
<Outlet /> | ||
cimigree marked this conversation as resolved.
Show resolved
Hide resolved
|
||
</Suspense> | ||
</div> | ||
</Paper> | ||
<div>map component here</div> | ||
<div>parent map component render count: {renderCount.current}</div> | ||
</div> | ||
<Suspense fallback={<CircularProgress />}> | ||
<div>map component here</div> | ||
</Suspense> | ||
</Container> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This component seems very complex. It handles routing, tab rendering, Suspense and layout. Could there be a separate function or hook to handle the logic of determining which tab is selected? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Im open to simplfying this. Im just not fully sure how we do that. Do you have any suggestions? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe a useMapTabs hook to determine what tab to show? But maybe that is unnecessary. Just a suggestion. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So, because the tabs needed a I think this brings up the question of what is the scope of a component. Gregor has argued that the parent component should contain all the logic, and while it is messier, it does expose everything. And while i don't fully agree, he has some good points. I think there needs to be a more in depth conversation about what are the standards for components. Regardless, I think the optimization of encapsulating the logic makes sense here because navigation touches everything, and this is a layout component that is the parent for the rest of the app. So the optimization is a necessary as opposed to an over optimization. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it does too. Thanks. I think we will be glad of this later. |
||
) | ||
} | ||
|
||
type TabProps = React.ComponentProps<typeof Tab> | ||
|
||
type MapTabRoute = { | ||
[K in keyof FileRoutesById]: K extends `${'/(MapTabs)/_Map'}${infer Rest}` | ||
? Rest extends '' | ||
? never | ||
: `${Rest}` | ||
: never | ||
}[keyof FileRoutesById] | ||
|
||
type MapTabProps = Omit<TabProps, 'value'> & { value: MapTabRoute } | ||
|
||
function MapTab(props: MapTabProps) { | ||
return <Tab {...props} /> | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,7 +1,8 @@ | ||
import { Suspense } from 'react' | ||
import { CssBaseline, ThemeProvider } from '@mui/material' | ||
import CircularProgress from '@mui/material/CircularProgress' | ||
import { QueryClient, QueryClientProvider } from '@tanstack/react-query' | ||
import { Outlet, createRootRoute } from '@tanstack/react-router' | ||
import { TanStackRouterDevtools } from '@tanstack/router-devtools' | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do you want to remove the actual Devtools also? It is commented out right now. |
||
|
||
import { theme } from '../Theme' | ||
import { ApiProvider } from '../contexts/ApiContext' | ||
|
@@ -16,8 +17,9 @@ export const Route = createRootRoute({ | |
<IntlProvider> | ||
<QueryClientProvider client={queryClient}> | ||
<ApiProvider> | ||
<Outlet /> | ||
<TanStackRouterDevtools /> | ||
<Suspense fallback={<CircularProgress />}> | ||
<Outlet /> | ||
</Suspense> | ||
</ApiProvider> | ||
</QueryClientProvider> | ||
</IntlProvider> | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a bunch of spacing was being multipied by 8 because of the way the theme api. So setting the spacing to 1 means any padding or margin is multiplied by one. I don't think we have any utility in one defined default spacing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good find! Sorry about that. I probably added that back in the summer and totally forgot why!