Fixing Header to rely on location and not parameter. #6472
Conversation
…of the parameter abValue which does not exist.
✅ Deploy Preview for inventree-web-pui-preview ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
|
^ The |
|
I see; it was telling me what to change, not what the issue was. Got it! |
matmair
added
bug
| @@ -100,12 +100,13 @@ export function Header() { | |||
|
|
|||
| function NavTabs() { | |||
| const { classes } = InvenTreeStyle(); | |||
| const { tabValue } = useParams(); | |||
| const defaultTabValue = 'home'; | |||
| const tabValue = useLocation().pathname.split('/')[1] || defaultTabValue; | |||
There was a problem hiding this comment.
Not sure that is a great solution, this is indirectly hardcoded to the current schema - which might change at any point. Thoughts @wolflu05 ?
There was a problem hiding this comment.
Yes, this would also be my first concern, as we have the feature to configure a relative path where PUI is hosted. Maybe we can use that setting to somehow strip the prefix or find a solution to only get the url from after the prefix.
And secondly it would be better to wrap the .split and parsing of the panel name into a useMemo hook, so this is in a state and does not cause a rerendering.
There was a problem hiding this comment.
I agree with your concerns, and it may not be the best idea for this application. If you do come up with a solution, let me know (@ me). I would love to see how you guys fix it!
There was a problem hiding this comment.
Another method I tried (and failed to get to work) was to replace the path in the top-level route (<Route path="/" element={<LayoutComponent />} errorElement={<ErrorPage />}>) with something like path="/tabValue?", so the value of the child roots would be filled into that value, and theoretically still work, but in practice, it either absorbs everything, or breaks children routing.
It might be worth a shot? I'm not qualified enough in React to know if it's possible.
|
How important is it that the current "top level tab" is indicated in this fashion? There are many pages (e.g. settings / notifications / etc) which do not even fit under this scheme. The component makes sense for switching between content within a single page, but maybe feels like the wrong solution for global path navigation... Thoughts? |
|
Personally I really like this navigation to switch between the core parts of InvenTree and would miss it if it would be removed. |
|
To be clear I am not talking about removing the tabs themselves - just the "current tab" indicator (unless we can work out a good way to implement) |
|
I am bumping this to |
|
@CodingPupper3033 could you adress the merge conflict so we can get this into 0.16.0? |
Closes #6471
Fixes issue #6471
Let me check the commit test server thing to see if it's working before it's merged.