chore: style navigation tabs #57

ErikSin · 2024-12-11T23:31:04Z

Styled the navigation tabs to prep for the map component.

cimigree

Is this related to this issue? chore-style-tabs

I am getting tons of errors / warnings both in the terminal when running tests and the developer console (especially when clicking on the tabs) These relate to invalid props on Dom elements (fullWidth, indicator, selectionFollowsFocus, and textColor) that we are not using explicitly so not sure what is going on here. Did you look into it? It seems like it is something we should try to fix. If for no other reason than that any [other] errors we need to address will be invisible among all the other noise.

The other warning I get repeatedly is:
chunk-GYPSVNDI.js?v=08559733:674 MUI: The value provided to the Tabs component is invalid.
None of the Tabs' children match with "undefined".
You can provide one of the following values: /tab1, 1, /tab2.

Also, I don't seem to be passing through the index file at all.
src/renderer/src/routes/index.tsx
Do you know why?
I thought I would see the onboarding screens but I am not.

I understand that this is a foundation, but I wonder if it could be cleaned up and clarified a little more before it is merged based on some of the comments below and in here.

src/renderer/src/routes/(MapTabs)/_Map.tsx

cimigree · 2024-12-12T18:04:51Z

src/renderer/src/routes/(MapTabs)/_Map.tsx

+							</Text>
+						}
+						value={'/tab2'}
+					/>


If you add an sx here with paddingBottom the about icon won't be so close to the bottom of the screen...

src/renderer/src/routes/(MapTabs)/_Map.tab1.tsx

src/renderer/src/routes/(MapTabs)/_Map.tab2.tsx

cimigree · 2024-12-12T18:07:19Z

src/renderer/src/routes/(MapTabs)/_Map.tsx

+						icon={<PostAddIcon />}
+						value={'/tab1'}
+					/>
+					<Box flexGrow={1} />


This is kind of unfortunate that you had to do this to put the tabs at the bottom. Just noticing. Nothing I am suggesting you change.

THIS BOX IS CAUSING ALL THE CONSOLE ERRORS

cimigree · 2024-12-12T18:10:07Z

src/renderer/src/routes/(MapTabs)/_Map.tsx

-		</div>
+			<Suspense fallback={<CircularProgress />}>
+				<div>map component here</div>
+			</Suspense>


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

Im open to simplfying this. Im just not fully sure how we do that. Do you have any suggestions?

Maybe a useMapTabs hook to determine what tab to show? But maybe that is unnecessary. Just a suggestion.

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.

I think it does too. Thanks. I think we will be glad of this later.

src/renderer/src/routes/(MapTabs)/_Map.tsx

ErikSin · 2024-12-12T18:32:07Z

Is this related to this issue? chore-style-tabs

I am getting tons of errors / warnings both in the terminal when running tests and the developer console (especially when clicking on the tabs) These relate to invalid props on Dom elements (fullWidth, indicator, selectionFollowsFocus, and textColor) that we are not using explicitly so not sure what is going on here. Did you look into it? It seems like it is something we should try to fix. If for no other reason than that any [other] errors we need to address will be invisible among all the other noise.

Oooh, i didnt open up my console so i didnt see any of these. Will look into them now

ErikSin · 2024-12-12T21:08:32Z

I fixed all the errors. I had to do some hacky things BUT i think these hacky things are better than the alternatives.

to deal with the spacing of the tabs, i was originally using a < box/>. This box was causing all the console errors because the tabs was trying to pass it props that didn't exist. So I replaced it with a < Tab />, disabled it, and didnt give it a label or icon. Its techincally in the react tree, but it is not seen or interact-able. I couldn't think of another alternative. The api just doesn't allow for the styling customization that we want
the / route technically return null as the component. But, the useLayoutEffect redirects the user to the approriate page. While this is not ideal code, the other alternative is to use the beforeLoading prop in tanstack. But the router then needs use of the hook useDeviceInfo to determine where to navigate too. In order to pass this into the router, we need to use a the router provider which needs to initialized on creation of the router. Its quite a lot of complex code for a simple redirect. I think this is a better and cleaner alternative.

ErikSin · 2024-12-12T21:24:04Z

src/renderer/src/Theme.ts

@@ -86,6 +86,7 @@ const theme = createTheme({
 			},
 		},
 	},
+	spacing: 1,


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

Good find! Sorry about that. I probably added that back in the summer and totally forgot why!

cimigree

Nice work fixing all of those errors! Very impressive!

cimigree · 2024-12-12T21:40:25Z

src/renderer/src/routes/__root.tsx

@@ -3,7 +3,6 @@ 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'


Do you want to remove the actual Devtools also? It is commented out right now.

cimigree · 2024-12-12T21:44:21Z

src/renderer/src/routes/(MapTabs)/_Map.tsx

-		</div>
+			<Suspense fallback={<CircularProgress />}>
+				<div>map component here</div>
+			</Suspense>


I think it does too. Thanks. I think we will be glad of this later.

ErikSin added 7 commits December 10, 2024 12:57

chore: update root to use suspense

46c8446

chore: update Map Layout

36d5b15

chore: styled map tabs

35ddc9c

chore: testing

dbda710

scaffold test

04ca16e

Merge branch 'main' into chore-style-tabs

f7af168

chore: adds routing tests

df44dd2

ErikSin requested a review from cimigree December 11, 2024 23:32

cimigree requested changes Dec 12, 2024

View reviewed changes

ErikSin added 2 commits December 12, 2024 12:40

chore: update tabs to remove console logs

a9e9aa0

chore: update index route to redirect in useLayoutEffect

e330b43

ErikSin requested a review from cimigree December 12, 2024 21:08

ErikSin added 4 commits December 12, 2024 13:13

chore: remove unnecessary styling

a8a5b22

chore: padding

bcdb942

chore: removed TanStackRouterDevtool

1fe3d9e

chore: remove import

68636fa

ErikSin commented Dec 12, 2024

View reviewed changes

Merge branch 'main' into chore-style-tabs

e6c7b71

cimigree approved these changes Dec 12, 2024

View reviewed changes

chore: remove dev tool

c67a298

ErikSin merged commit 4b6302f into main Dec 13, 2024
2 checks passed

ErikSin deleted the chore-style-tabs branch December 13, 2024 02:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: style navigation tabs #57

chore: style navigation tabs #57

ErikSin commented Dec 11, 2024

cimigree left a comment •

edited

Loading

cimigree Dec 12, 2024

ErikSin Dec 12, 2024

cimigree Dec 12, 2024 •

edited

Loading

ErikSin Dec 12, 2024

cimigree Dec 12, 2024

ErikSin Dec 12, 2024

cimigree Dec 12, 2024

ErikSin Dec 12, 2024

cimigree Dec 12, 2024

ErikSin commented Dec 12, 2024

ErikSin commented Dec 12, 2024 •

edited

Loading

ErikSin Dec 12, 2024

cimigree Dec 12, 2024

cimigree left a comment

cimigree Dec 12, 2024

cimigree Dec 12, 2024

chore: style navigation tabs #57

chore: style navigation tabs #57

Conversation

ErikSin commented Dec 11, 2024

cimigree left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cimigree Dec 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ErikSin commented Dec 12, 2024

ErikSin commented Dec 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cimigree left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cimigree left a comment •

edited

Loading

cimigree Dec 12, 2024 •

edited

Loading

ErikSin commented Dec 12, 2024 •

edited

Loading