-
Notifications
You must be signed in to change notification settings - Fork 3
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
Development to Main, week 40 #114
Changes from all commits
bd509b1
0e6df19
997222f
757d765
130a605
8b6beb7
bf68180
6d98186
3de1a7c
f2e4d35
70f9e36
ddc3c4c
1a40ac5
3334c8d
00f2c0f
367b8eb
74d94c3
a31047e
f492e7e
66c8723
9c5d9df
5c48485
5e0deff
7cacaef
7e7e899
614a469
adf42a3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,7 +3,7 @@ import * as styles from "./PrimaryTopNav.module.css"; | |
import clsx from "clsx"; | ||
import { Link } from "@utrecht/component-library-react/dist/css-module"; | ||
import { FontAwesomeIcon } from "@fortawesome/react-fontawesome"; | ||
import { faBars } from "@fortawesome/free-solid-svg-icons"; | ||
import { faBars, faChevronRight } from "@fortawesome/free-solid-svg-icons"; | ||
|
||
interface ITopNavItem { | ||
label: string; | ||
|
@@ -26,6 +26,7 @@ export interface TopNavProps { | |
|
||
export const PrimaryTopNav = ({ items, mobileLogo, layoutClassName }: TopNavProps): JSX.Element => { | ||
const [isOpen, setIsOpen] = React.useState<boolean>(false); | ||
const [subNavIsOpen, setSubNavIsOpen] = React.useState({}); | ||
|
||
const handleItemClick = (handleClick?: () => any) => { | ||
if (handleClick) { | ||
|
@@ -35,6 +36,20 @@ export const PrimaryTopNav = ({ items, mobileLogo, layoutClassName }: TopNavProp | |
} | ||
}; | ||
|
||
const handleSubItemMenu = (idx: number, value: boolean) => { | ||
setSubNavIsOpen({ | ||
...subNavIsOpen, | ||
[idx as keyof typeof subNavIsOpen]: value, | ||
}); | ||
}; | ||
Comment on lines
+39
to
+44
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This seems like a complex way to keep track of the open sub items. Previously, I think, we just let the css handle everything. Why the change? |
||
|
||
const screenWidth = window.innerWidth; | ||
|
||
React.useEffect(() => { | ||
if (screenWidth > 992) { | ||
setSubNavIsOpen({}); | ||
} | ||
}, [screenWidth]); | ||
Comment on lines
+46
to
+52
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this is not triggering on the "right" moments. To accurately keep track of a screen width, we can register (and un-register when needed) a listener, like so (dummy code):
|
||
return ( | ||
<div className={clsx(styles.container, layoutClassName && layoutClassName)}> | ||
<div className={styles.menuToggleContainer}> | ||
|
@@ -49,24 +64,43 @@ export const PrimaryTopNav = ({ items, mobileLogo, layoutClassName }: TopNavProp | |
<ul className={styles.ul}> | ||
{items.map(({ label, icon, current, handleClick, subItems }, idx) => ( | ||
<li | ||
onClick={() => handleItemClick(handleClick)} | ||
onClick={() => | ||
!subItems | ||
? handleItemClick(handleClick) | ||
: screenWidth > 992 | ||
? handleItemClick(handleClick) | ||
: handleSubItemMenu(idx, !subNavIsOpen[idx as keyof typeof subNavIsOpen] ?? true) | ||
} | ||
className={clsx(styles.li, current && styles.current)} | ||
key={idx} | ||
> | ||
<Link className={clsx(styles.link, styles.label)}> | ||
<Link | ||
className={clsx( | ||
styles.link, | ||
styles.label, | ||
subItems && styles.mobileLink, | ||
current && styles.currentLink, | ||
)} | ||
> | ||
{icon} | ||
{label} | ||
{label}{" "} | ||
{subItems && screenWidth < 992 && ( | ||
<FontAwesomeIcon | ||
className={clsx(styles.toggleIcon, subNavIsOpen[idx as keyof typeof subNavIsOpen] && styles.isOpen)} | ||
icon={faChevronRight} | ||
/> | ||
)} | ||
Comment on lines
+67
to
+92
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I find this a bit complex, see my other comment as well. Is this because a top-level item can have a link and have sub-items? |
||
</Link> | ||
|
||
{subItems && ( | ||
<ul className={styles.dropdown}> | ||
<ul className={clsx(styles.dropdown, subNavIsOpen[idx as keyof typeof subNavIsOpen] && styles.isOpen)}> | ||
{subItems.map(({ label, icon, current, handleClick }, idx) => ( | ||
<li | ||
key={idx} | ||
className={clsx(styles.li, current && styles.current)} | ||
onClick={() => handleItemClick(handleClick)} | ||
> | ||
<Link className={clsx(styles.link, styles.label)}> | ||
<Link className={clsx(styles.link, styles.label, current && styles.currentLink)}> | ||
{icon} | ||
{label} | ||
</Link> | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should type this state, it's unclear what it's expecting.