More route nav id fixe #2
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Summary
This PR introduces significant changes to Grafana's navigation system, focusing on vertical navigation and page layout components.
- Added new
VerticalTabcomponent in/packages/grafana-ui/src/components/Tabs/VerticalTab.tsxwith support for icons, counters, and themed styling - Introduced new
PageNewcomponents in/public/app/core/components/PageNew/includingPage.tsx,PageHeader.tsx,PageTabs.tsx, andSectionNav.tsxfor enhanced navigation - Removed
breadcrumbsproperty fromNavModelinterface in favor of item-level breadcrumbs throughNavModelItem.breadcrumbs - Added new hooks
usePageNavandusePageTitlein/public/app/core/components/Page/for centralized navigation state management - Renamed
subNavprop topageNavacross multiple components for consistent navigation terminology
29 file(s) reviewed, 25 comment(s)
Edit PR Review Bot Settings | Greptile
| } | ||
| ); | ||
|
|
||
| VerticalTab.displayName = 'Tab'; |
There was a problem hiding this comment.
style: displayName 'Tab' may be confusing since this is VerticalTab component
| }; | ||
|
|
||
| describe('Render', () => { | ||
| it('should render component with emtpy Page container', async () => { |
There was a problem hiding this comment.
syntax: 'emtpy' is misspelled in test description
| document.title = Branding.AppTitle; | ||
| } | ||
| }, [navModel]); | ||
| const pageHeaderNav = pageNav ?? navModel?.main; |
There was a problem hiding this comment.
logic: navModel?.main could be undefined even when navModel exists, which would hide the header unexpectedly
| if (!navId) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
style: return undefined instead of empty return for consistency
| // eslint-disable-next-line react-hooks/rules-of-hooks | ||
| return useSelector(createSelector(getNavIndex, (navIndex) => getNavModel(navIndex, navId ?? 'home'))); |
There was a problem hiding this comment.
logic: useSelector hook is conditionally called, which violates React hooks rules. Consider restructuring to avoid conditional hook usage
| ); | ||
| })} | ||
| {nestedItems.map((child) => ( | ||
| <> |
There was a problem hiding this comment.
logic: Fragment key missing for mapped array elements
| const directChildren = props.model.main.children!.filter((x) => !x.hideFromTabs && !x.children); | ||
| const nestedItems = props.model.main.children!.filter((x) => x.children && x.children.length); |
There was a problem hiding this comment.
logic: Non-null assertion operator (!) used on children property could cause runtime errors if main.children is undefined
| return ( | ||
| !child.hideFromTabs && | ||
| !child.children && ( | ||
| <VerticalTab | ||
| label={child.text} | ||
| active={child.active} | ||
| key={`${child.url}-${index}`} | ||
| // icon={child.icon as IconName} | ||
| href={child.url} | ||
| /> | ||
| ) | ||
| ); | ||
| })} |
There was a problem hiding this comment.
style: Redundant hideFromTabs and children checks - already filtered in directChildren definition
| return ( | ||
| <div className={styles.pageToolbar}> | ||
| <div className={styles.menuButton}> | ||
| <IconButton name="bars" tooltip="Toggle menu" tooltipPlacement="bottom" size="xl" onClick={() => {}} /> |
There was a problem hiding this comment.
logic: Empty onClick handler will make menu button non-functional
| sectionNav={navModel.node} | ||
| pageNav={props.pageNav} |
There was a problem hiding this comment.
style: pageNav is now passed through twice - both via {...props} spread and explicitly. Consider removing explicit prop
What is this feature?
[Add a brief description of what the feature or update does.]
Why do we need this feature?
[Add a description of the problem the feature is trying to solve.]
Who is this feature for?
[Add information on what kind of user the feature is for.]
Which issue(s) does this PR fix?:
Fixes #
Special notes for your reviewer:
Please check that: