chore: refactor nav components #598 #599
Conversation
ursucarina
requested review from
a team,
eugenejahn,
jsonporter and
james-union
and removed request for
a team
Codecov Report
@@ Coverage Diff @@
## master #599 +/- ##
==========================================
+ Coverage 69.08% 69.26% +0.18%
==========================================
Files 440 442 +2
Lines 10434 10450 +16
Branches 1789 1790 +1
==========================================
+ Hits 7208 7238 +30
+ Misses 3226 3212 -14
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
Signed-off-by: Carina Ursu <[email protected]>
Signed-off-by: Carina Ursu <[email protected]>
ursucarina
force-pushed
the
carina/navdropdown
branch
from
d1f74c2 to
c41dd85
Compare
| export const DefaultAppBarContent = (props: DefaultAppBarProps) => { | ||
| const { console, items } = props; |
There was a problem hiding this comment.
nit
| export const DefaultAppBarContent = (props: DefaultAppBarProps) => { | |
| const { console, items } = props; | |
| export const DefaultAppBarContent = ({ console, items }: DefaultAppBarProps) => { |
| url: 'https://github.com/flyteorg/flyteconsole#google-analytics', | ||
| }, | ||
| ]; | ||
| const versions: VersionInfo[] = React.useMemo( |
There was a problem hiding this comment.
nit:
move to import {useMemo} from 'react'; - I'm trying to get us to this way of importing standard hooks, as sometimes it saves lines 😆 not in this case, but a lot of state definitions became more readable as one-liners IMO
| const defaultButton = getByRole('button'); | ||
| expect(defaultButton).not.toBeNull(); |
There was a problem hiding this comment.
nit: I think switching to queryBy is better to check presence of an element, as it returns null if element is not found, while get throws an error
| export const NavigationDropdown = (props: NavigationDropdownProps) => { | ||
| // Flyte Console list item - always there ans is first in the list | ||
| const ConsoleItem: FlyteNavItem = React.useMemo(() => { | ||
| return { | ||
| title: props.console ?? 'Console', | ||
| url: makeRoute('/'), | ||
| }; | ||
| }, [props.console]); | ||
| const { baseUrl, items: menuItems, config } = props; |
There was a problem hiding this comment.
nit: destructure props on definition
export const NavigationDropdown = ({ baseUrl, items: menuItems, config }: NavigationDropdownProps) => {
|
|
||
| const [selectedPage, setSelectedPage] = React.useState<string>(ConsoleItem.title); | ||
| const basePathName = React.useMemo(() => getBasePathName(), []); |
There was a problem hiding this comment.
nit: update useState and useMemo imports
import {useMemo, useState} from 'react';
| export const makeRoute = (baseUrl: string, path: string) => `${baseUrl}${path}`; | ||
|
|
||
| export const getBasePathName = () => { | ||
| const pathName = window.location.pathname; | ||
|
|
||
| return `/${pathName?.split('/')?.[1]}`; | ||
| }; |
There was a problem hiding this comment.
nit:
should this folder's name be capitalized (Utils)? it's not gonna be a component, and seems out of pattern with other non-component folders, ie routes, touched in this PR
Signed-off-by: Carina Ursu [email protected]
TL;DR
Refactor of Navigation Dropdown into its own component
Type
Are all requirements met?
Complete description
Refactor of Navigation Dropdown into a plugin
Tracking Issue
flyteorg/flyte#598
Follow-up issue
flyteorg/flyte#598