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

chore: style navigation tabs #57

Merged
merged 15 commits into from
Dec 13, 2024
6 changes: 6 additions & 0 deletions messages/renderer/en.json
Original file line number Diff line number Diff line change
Expand Up @@ -238,5 +238,11 @@
},
"screens.ProjectCreationScreen.title": {
"message": "Create a Project"
},
"tabBar.label.about": {
"message": "About"
},
"tabBar.label.settings": {
"message": "Settings"
}
}
15 changes: 15 additions & 0 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions src/renderer/src/Theme.ts
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,7 @@ const theme = createTheme({
},
},
},
spacing: 1,
Copy link
Contributor Author

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

Copy link
Contributor

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!

})

export { theme }
91 changes: 91 additions & 0 deletions src/renderer/src/components/Tabs.tsx
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} />
}
8 changes: 6 additions & 2 deletions src/renderer/src/queries/deviceInfo.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,8 @@
import { useMutation, useQuery, useQueryClient } from '@tanstack/react-query'
import {
useMutation,
useQueryClient,
useSuspenseQuery,
} from '@tanstack/react-query'

import { useApi } from '../contexts/ApiContext'

Expand All @@ -7,7 +11,7 @@ export const DEVICE_INFO_KEY = 'deviceInfo'
export const useDeviceInfo = () => {
const api = useApi()

return useQuery({
return useSuspenseQuery({
queryKey: [DEVICE_INFO_KEY],
queryFn: async () => {
return await api.getDeviceInfo()
Expand Down
4 changes: 2 additions & 2 deletions src/renderer/src/routes/(MapTabs)/_Map.tab1.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,10 @@ import { createFileRoute } from '@tanstack/react-router'
import { Text } from '../../components/Text'

export const Route = createFileRoute('/(MapTabs)/_Map/tab1')({
component: RouteComponent,
component: Observations,
cimigree marked this conversation as resolved.
Show resolved Hide resolved
})

function RouteComponent() {
export function Observations() {
return (
<div>
<Text>Tab 1</Text>
Expand Down
4 changes: 2 additions & 2 deletions src/renderer/src/routes/(MapTabs)/_Map.tab2.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,10 @@ import { createFileRoute } from '@tanstack/react-router'
import { Text } from '../../components/Text'

export const Route = createFileRoute('/(MapTabs)/_Map/tab2')({
component: RouteComponent,
component: Settings,
cimigree marked this conversation as resolved.
Show resolved Hide resolved
})

function RouteComponent() {
export function Settings() {
return (
<div>
<Text>Tab 2</Text>
Expand Down
64 changes: 49 additions & 15 deletions src/renderer/src/routes/(MapTabs)/_Map.test.tsx
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')
})
69 changes: 31 additions & 38 deletions src/renderer/src/routes/(MapTabs)/_Map.tsx
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>
Copy link
Contributor

Choose a reason for hiding this comment

The 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?
It would be nice if MapLayout could just focus on Layout

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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?

Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So, because the tabs needed a value prop it had to be aware of the navigation name. This requires the use of useLocation hook. This was causing the entire layout screen and all its children (aka the whole app) to rerender on every change in location. So I had to encapsulate the tabs into its own component. I think it simplifies the component?

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.

Copy link
Contributor

Choose a reason for hiding this comment

The 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} />
}
8 changes: 5 additions & 3 deletions src/renderer/src/routes/__root.tsx
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'
Copy link
Contributor

Choose a reason for hiding this comment

The 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'
Expand All @@ -16,8 +17,9 @@ export const Route = createRootRoute({
<IntlProvider>
<QueryClientProvider client={queryClient}>
<ApiProvider>
<Outlet />
<TanStackRouterDevtools />
<Suspense fallback={<CircularProgress />}>
<Outlet />
</Suspense>
</ApiProvider>
</QueryClientProvider>
</IntlProvider>
Expand Down
Loading
Loading