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

More route nav id fixe #2

Open
wants to merge 17 commits into
base: main
Choose a base branch
from
Open

More route nav id fixe #2

wants to merge 17 commits into from

Conversation

lizard-boy
Copy link

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:

  • It works as expected from a user's perspective.
  • If this is a pre-GA feature, it is behind a feature toggle.
  • The docs are updated, and if this is a notable improvement, it's added to our What's New doc.

Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

PR Summary

This PR introduces significant changes to Grafana's navigation system, focusing on vertical navigation and page layout components.

  • Added new VerticalTab component in /packages/grafana-ui/src/components/Tabs/VerticalTab.tsx with support for icons, counters, and themed styling
  • Introduced new PageNew components in /public/app/core/components/PageNew/ including Page.tsx, PageHeader.tsx, PageTabs.tsx, and SectionNav.tsx for enhanced navigation
  • Removed breadcrumbs property from NavModel interface in favor of item-level breadcrumbs through NavModelItem.breadcrumbs
  • Added new hooks usePageNav and usePageTitle in /public/app/core/components/Page/ for centralized navigation state management
  • Renamed subNav prop to pageNav across multiple components for consistent navigation terminology

29 file(s) reviewed, 25 comment(s)
Edit PR Review Bot Settings | Greptile

}
);

VerticalTab.displayName = 'Tab';
Copy link

Choose a reason for hiding this comment

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

style: displayName 'Tab' may be confusing since this is VerticalTab component

};

describe('Render', () => {
it('should render component with emtpy Page container', async () => {
Copy link

Choose a reason for hiding this comment

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

syntax: 'emtpy' is misspelled in test description

document.title = Branding.AppTitle;
}
}, [navModel]);
const pageHeaderNav = pageNav ?? navModel?.main;
Copy link

Choose a reason for hiding this comment

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

logic: navModel?.main could be undefined even when navModel exists, which would hide the header unexpectedly

Comment on lines +14 to +16
if (!navId) {
return;
}
Copy link

Choose a reason for hiding this comment

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

style: return undefined instead of empty return for consistency

Comment on lines +23 to +24
// eslint-disable-next-line react-hooks/rules-of-hooks
return useSelector(createSelector(getNavIndex, (navIndex) => getNavModel(navIndex, navId ?? 'home')));
Copy link

Choose a reason for hiding this comment

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

logic: useSelector hook is conditionally called, which violates React hooks rules. Consider restructuring to avoid conditional hook usage

);
})}
{nestedItems.map((child) => (
<>
Copy link

Choose a reason for hiding this comment

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

logic: Fragment key missing for mapped array elements

Comment on lines +15 to +16
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);
Copy link

Choose a reason for hiding this comment

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

logic: Non-null assertion operator (!) used on children property could cause runtime errors if main.children is undefined

Comment on lines +27 to +39
return (
!child.hideFromTabs &&
!child.children && (
<VerticalTab
label={child.text}
active={child.active}
key={`${child.url}-${index}`}
// icon={child.icon as IconName}
href={child.url}
/>
)
);
})}
Copy link

Choose a reason for hiding this comment

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

style: Redundant hideFromTabs and children checks - already filtered in directChildren definition

}

export function NavToolbar({ actions, onToggleSearchBar, searchBarHidden, sectionNav, subNav }: Props) {
export function NavToolbar({ actions, onToggleSearchBar, searchBarHidden, sectionNav, pageNav }: Props) {
const styles = useStyles2(getStyles);

return (
<div className={styles.pageToolbar}>
<div className={styles.menuButton}>
<IconButton name="bars" tooltip="Toggle menu" tooltipPlacement="bottom" size="xl" onClick={() => {}} />
Copy link

Choose a reason for hiding this comment

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

logic: Empty onClick handler will make menu button non-functional

Comment on lines 38 to +39
sectionNav={navModel.node}
pageNav={props.pageNav}
Copy link

Choose a reason for hiding this comment

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

style: pageNav is now passed through twice - both via {...props} spread and explicitly. Consider removing explicit prop

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants