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

[Menubar] Base implementation of refactored NavBar #3279

Open
wants to merge 35 commits into
base: develop
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
35 commits
Select commit Hold shift + click to select a range
b16dada
fix: renamed files, components, imports, exports, contexts, and methods
tespin Nov 16, 2024
0f9cf65
refactor: separated UserMenu from the menubar component, wrapped both…
tespin Nov 18, 2024
ab818ad
style: updated header and menubar styles
tespin Nov 18, 2024
246597f
refactor: moved LanguageMenu into left items
tespin Nov 18, 2024
9f3096c
refactor: split new MenubarMenu component into Trigger, List, and Men…
tespin Nov 19, 2024
7c3b896
test: updated snapshots
tespin Nov 19, 2024
b2eb033
refactor: removed menuitem roles from sign up and login, added role=p…
tespin Nov 19, 2024
06e9152
fix: lint fixes
tespin Nov 19, 2024
905ac20
chore: added separators between components within MenubarMenu
tespin Nov 19, 2024
30f2c1b
Merge branch 'develop' into tespin/refactor-navbar-base
tespin Nov 19, 2024
b8a45d5
Fix#3241 Icon Alligment issue in Password Field
Nov 2, 2024
dbc98dd
added some fixes on the eye-svg
Nov 23, 2024
72daf4d
missing dispatch import
raclim Nov 26, 2024
e5e2ec1
wrap actions with dispatch
raclim Nov 27, 2024
f122ec6
update test to add useDispatch
raclim Nov 27, 2024
5c8a912
update dispatch and add selectors
raclim Nov 28, 2024
2177976
create parseFileName util
raclim Nov 28, 2024
0e9a967
update FileNode tests
raclim Nov 28, 2024
fd3e3a6
update FileNode import
raclim Nov 28, 2024
48f9ad8
added donation banner
Stefterv Nov 30, 2024
f84ebd9
2.15.4
raclim Dec 3, 2024
a361329
Revert "Fix Console Errors and Update Hooks in FileNode"
raclim Dec 3, 2024
b63c031
add back in consoleinput changes
raclim Dec 3, 2024
c8118f6
2.15.5
raclim Dec 3, 2024
16644f3
add apple pay id
raclim Dec 4, 2024
9444470
remove static folder
raclim Dec 4, 2024
060b13a
actually delete the static folder
raclim Dec 4, 2024
096edb8
move to public folder
raclim Dec 4, 2024
89a81d8
ensure content type is set correctly in server
raclim Dec 4, 2024
41b10f9
set absolute path
raclim Dec 4, 2024
50077b6
explicitly set root path
raclim Dec 4, 2024
043eb44
remove apple pay setup in server
raclim Dec 5, 2024
71fd59e
2.15.6
raclim Dec 5, 2024
d42872f
Remove Icon from README.md
raclim Dec 6, 2024
4d2ae14
Merge branch 'develop' into tespin/refactor-navbar-base
tespin Dec 13, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
86 changes: 86 additions & 0 deletions client/components/Menubar/Menubar.jsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,86 @@
import PropTypes from 'prop-types';
import React, { useCallback, useMemo, useRef, useState } from 'react';
import useModalClose from '../../common/useModalClose';
import { MenuOpenContext, MenubarContext } from './contexts';

function Menubar({ children, className }) {
const [menuOpen, setMenuOpen] = useState('none');

const timerRef = useRef(null);

const handleClose = useCallback(() => {
setMenuOpen('none');
}, [setMenuOpen]);

const nodeRef = useModalClose(handleClose);

const clearHideTimeout = useCallback(() => {
if (timerRef.current) {
clearTimeout(timerRef.current);
timerRef.current = null;
}
}, [timerRef]);

const handleBlur = useCallback(() => {
timerRef.current = setTimeout(() => setMenuOpen('none'), 10);
}, [timerRef, setMenuOpen]);

const toggleMenuOpen = useCallback(
(menu) => {
setMenuOpen((prevState) => (prevState === menu ? 'none' : menu));
},
[setMenuOpen]
);

const contextValue = useMemo(
() => ({
createMenuHandlers: (menu) => ({
onMouseOver: () => {
setMenuOpen((prevState) => (prevState === 'none' ? 'none' : menu));
},
onClick: () => {
toggleMenuOpen(menu);
},
onBlur: handleBlur,
onFocus: clearHideTimeout
}),
createMenuItemHandlers: (menu) => ({
onMouseUp: (e) => {
if (e.button === 2) {
return;
}
setMenuOpen('none');
},
onBlur: handleBlur,
onFocus: () => {
clearHideTimeout();
setMenuOpen(menu);
}
}),
toggleMenuOpen
}),
[setMenuOpen, toggleMenuOpen, clearHideTimeout, handleBlur]
);

return (
<MenubarContext.Provider value={contextValue}>
<div className={className} ref={nodeRef}>
<MenuOpenContext.Provider value={menuOpen}>
{children}
</MenuOpenContext.Provider>
</div>
</MenubarContext.Provider>
);
}

Menubar.propTypes = {
children: PropTypes.node,
className: PropTypes.string
};

Menubar.defaultProps = {
children: null,
className: 'nav'
};

export default Menubar;
Original file line number Diff line number Diff line change
@@ -1,12 +1,12 @@
import PropTypes from 'prop-types';
import React, { useContext, useMemo } from 'react';
import ButtonOrLink from '../../common/ButtonOrLink';
import { NavBarContext, ParentMenuContext } from './contexts';
import { MenubarContext, ParentMenuContext } from './contexts';

function NavMenuItem({ hideIf, className, ...rest }) {
function MenubarItem({ hideIf, className, ...rest }) {
const parent = useContext(ParentMenuContext);

const { createMenuItemHandlers } = useContext(NavBarContext);
const { createMenuItemHandlers } = useContext(MenubarContext);

const handlers = useMemo(() => createMenuItemHandlers(parent), [
createMenuItemHandlers,
Expand All @@ -24,7 +24,7 @@ function NavMenuItem({ hideIf, className, ...rest }) {
);
}

NavMenuItem.propTypes = {
MenubarItem.propTypes = {
...ButtonOrLink.propTypes,
onClick: PropTypes.func,
value: PropTypes.string,
Expand All @@ -35,11 +35,11 @@ NavMenuItem.propTypes = {
className: PropTypes.string
};

NavMenuItem.defaultProps = {
MenubarItem.defaultProps = {
onClick: null,
value: null,
hideIf: false,
className: 'nav__dropdown-item'
};

export default NavMenuItem;
export default MenubarItem;
100 changes: 100 additions & 0 deletions client/components/Menubar/MenubarMenu.jsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,100 @@
import classNames from 'classnames';
Copy link
Collaborator

Choose a reason for hiding this comment

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

I really, really love the changes here and the way this file is organized!

I know it's already within the tagged issue, but do you think it might be helpful to reference the article you've used as a comment here? Just came to mind since we have a few areas throughout the app that make these types of annotations!

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also found this article that touches on implementing focus traps for dropdown menus. Was wondering if you feel that might be applicable here, or if it might be unnecessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I really, really love the changes here and the way this file is organized!

I know it's already within the tagged issue, but do you think it might be helpful to reference the article you've used as a comment here? Just came to mind since we have a few areas throughout the app that make these types of annotations!

absolutely -- will add it as a comment so that it's clearer as a reference.

Also found this article that touches on implementing focus traps for dropdown menus. Was wondering if you feel that might be applicable here, or if it might be unnecessary?

thanks for this! i feel that focus trapping is something i associate more with modals and dialogs, where users can't tab out of the element unless a certain action is performed. i think we also want to be able to tab to the next element from the menubar without completely restricting focus in the way that we would with modals / dialogs / overlays. i'll see if i can find a menubar implementation in the wild that uses focus trapping, but here it might not be unnecessary.

that being said, we will definitely need a level of focus management to handle tabbing into the component as well as focusing various submenus / menuitems -- in the examples i've found, this has been done with a roving tabindex.

import PropTypes from 'prop-types';
import React, { useContext, useMemo } from 'react';
import TriangleIcon from '../../images/down-filled-triangle.svg';
import { MenuOpenContext, MenubarContext, ParentMenuContext } from './contexts';

export function useMenuProps(id) {
const activeMenu = useContext(MenuOpenContext);

const isOpen = id === activeMenu;

const { createMenuHandlers } = useContext(MenubarContext);

const handlers = useMemo(() => createMenuHandlers(id), [
createMenuHandlers,
id
]);

return { isOpen, handlers };
}

/* -------------------------------------------------------------------------------------------------
* MenubarTrigger
* -----------------------------------------------------------------------------------------------*/

function MenubarTrigger({ id, title, ...props }) {
const { isOpen, handlers } = useMenuProps(id);

return (
<button
{...handlers}
{...props}
role="menuitem"
aria-haspopup="menu"
aria-expanded={isOpen}
>
<span className="nav__item-header">{title}</span>
<TriangleIcon
className="nav__item-header-triangle"
focusable="false"
aria-hidden="true"
/>
</button>
);
Copy link
Collaborator

@raclim raclim Nov 26, 2024

Choose a reason for hiding this comment

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

Would aria-live also need to be added here, or is the current implementation sufficient?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

based on what i've read about aria-live, it's most used to announce dynamic updates to page content even if that area isn't focused on. i think a menubar with proper focus management and keyboard controls will allow screen readers to naturally announce any new changes or contexts, so there might not be a use for it here.

we could consider using aria-live in conjunction with the language menu, since selecting an option there updates the entire page. I can do some tests with a screen reader and see how that goes!

}

MenubarTrigger.propTypes = {
id: PropTypes.string.isRequired,
title: PropTypes.node.isRequired
};

/* -------------------------------------------------------------------------------------------------
* MenubarList
* -----------------------------------------------------------------------------------------------*/

function MenubarList({ id, children }) {
return (
<ul className="nav__dropdown" role="menu">
<ParentMenuContext.Provider value={id}>
{children}
</ParentMenuContext.Provider>
</ul>
);
}

MenubarList.propTypes = {
id: PropTypes.string.isRequired,
children: PropTypes.node
};

MenubarList.defaultProps = {
children: null
};

/* -------------------------------------------------------------------------------------------------
* MenubarMenu
* -----------------------------------------------------------------------------------------------*/

function MenubarMenu({ id, title, children }) {
const { isOpen } = useMenuProps(id);

return (
<li className={classNames('nav__item', isOpen && 'nav__item--open')}>
<MenubarTrigger id={id} title={title} />
<MenubarList id={id}>{children}</MenubarList>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would it maybe make sense to conditionally render the MenubarList when isOpen is true as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would it maybe make sense to conditionally render the MenubarList when isOpen is true as well?

great point. the wai aria menubar example doesn't conditionally render its list component, but I've seen component libraries handle this by rendering the component within a portal. definitely something that can be built in!

</li>
);
}

MenubarMenu.propTypes = {
id: PropTypes.string.isRequired,
title: PropTypes.node.isRequired,
children: PropTypes.node
};

MenubarMenu.defaultProps = {
children: null
};

export default MenubarMenu;
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,8 @@ export const ParentMenuContext = createContext('none');

export const MenuOpenContext = createContext('none');

export const NavBarContext = createContext({
createDropdownHandlers: () => ({}),
export const MenubarContext = createContext({
createMenuHandlers: () => ({}),
createMenuItemHandlers: () => ({}),
toggleDropdownOpen: () => {}
toggleMenuOpen: () => {}
});
92 changes: 0 additions & 92 deletions client/components/Nav/NavBar.jsx

This file was deleted.

59 changes: 0 additions & 59 deletions client/components/Nav/NavDropdownMenu.jsx

This file was deleted.

Loading
Loading