-
Notifications
You must be signed in to change notification settings - Fork 1
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
New Accessible Dropdown Nav Bar Component #478
base: main
Are you sure you want to change the base?
Changes from 1 commit
ac1c68a
60ffbf6
7b49707
1efbe42
bbc67b1
f360128
d0a4703
4b4a912
e048cd4
e9a3f52
a69dd10
0f058ed
e176440
c0c5f0b
f2af2b7
1d2222f
b3e5825
f9f79de
c2658ec
aa1e826
3c77519
a10e8cd
054ebdb
cf7d3b4
1c26012
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
- Loading branch information
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,4 @@ | ||
.dropdown-nav-bar { | ||
& { | ||
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 haven't seen this usage before! What does |
||
input[type='checkbox'] { | ||
position: absolute; | ||
top: -9999px; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
$tablet: new-breakpoint(min-width 0); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,8 @@ | ||
.dropdown-nav-bar.no-mobile { | ||
@import 'dropdown-nav-bar-no-mobile'; | ||
@import 'dropdown-nav-bar-common'; | ||
} | ||
|
||
.dropdown-nav-bar:not(.no-mobile) { | ||
@import 'dropdown-nav-bar-common'; | ||
} | ||
Comment on lines
+1
to
+8
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 allowed me to use the same styles for both implementations and just change the 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. Interesting! I'll defer to @marchwiany on best practices in using dynamic variables as this is pretty foreign to me. For my own edification, would the following not perform a similarly sequenced import? Maybe in the compilation step the second block is executed before the first? .dropdown-nav-bar.no-mobile {
@import 'dropdown-nav-bar-no-mobile';
}
.dropdown-nav-bar {
@import 'dropdown-nav-bar-common';
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,6 +3,7 @@ import PropTypes from 'prop-types' | |
import { isMobileView, toggleElementArray } from './helpers' | ||
import { menuItemType } from './helpers/nav-prop-types' | ||
import DropdownNavMenu from './dropdown-nav-menu' | ||
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. Is the intent to make all of the subcomponents exportable so that users can build their own nav bars, or is this the only file that's getting exported? It looks like it's the latter (based on |
||
import classnames from 'classnames' | ||
|
||
// TODO: Finish documentation | ||
/** | ||
|
@@ -12,7 +13,7 @@ import DropdownNavMenu from './dropdown-nav-menu' | |
* @type Function | ||
* @description A control component for navigating among multiple navigation menu items that can include dropdowns with sub-menu items | ||
* @param {String} [baseUrl] - | ||
* @param {Number} [mobileBreakpoint] - | ||
* @param {Number|Boolean} [mobileBreakpoint] - | ||
* @param {Array} [menuItems] - | ||
* @example | ||
* | ||
|
@@ -35,25 +36,25 @@ import DropdownNavMenu from './dropdown-nav-menu' | |
|
||
const propTypes = { | ||
menuItems: PropTypes.arrayOf(menuItemType).isRequired, | ||
mobileBreakpoint: PropTypes.number, | ||
mobileBreakpoint: PropTypes.oneOfType([PropTypes.number, PropTypes.bool]), | ||
baseUrl: PropTypes.string, | ||
menuLabel: PropTypes.string, | ||
hideDropdownButtonsBeforeFocus: PropTypes.bool, | ||
hideMenuButtonsBeforeFocus: PropTypes.bool, | ||
} | ||
|
||
const defaultProps = { | ||
mobileBreakpoint: 1024, | ||
mobileBreakpoint: 720, | ||
baseUrl: '', | ||
menuLabel: 'Primary Menu', | ||
hideDropdownButtonsBeforeFocus: false, | ||
hideMenuButtonsBeforeFocus: false, | ||
} | ||
|
||
function DropdownNavBar({ | ||
menuItems, | ||
mobileBreakpoint, | ||
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. Did you explore other alternatives to using a pixel value? How does this compare to other component libraries like Semantic UI, Bootstrap, etc.? 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. Can this value be configured via css instead of within the React component? I'm sure there's good reasoning for including it here I just haven't connected the dots yet |
||
baseUrl, | ||
menuLabel, | ||
hideDropdownButtonsBeforeFocus, | ||
hideMenuButtonsBeforeFocus, | ||
}) { | ||
const [isMobileMenuOpen, setIsMobileMenuOpen] = useState(false) | ||
const [openMenuIds, setOpenMenuIds] = useState([]) | ||
|
@@ -71,29 +72,38 @@ function DropdownNavBar({ | |
} | ||
|
||
return ( | ||
<nav className="dropdown-nav-bar" aria-label={menuLabel}> | ||
<input | ||
type="checkbox" | ||
id="mobile-nav-button" | ||
checked={isMobileMenuOpen} | ||
onChange={() => { | ||
setIsMobileMenuOpen(!isMobileMenuOpen) | ||
closeDesktopSubmenu() | ||
}} | ||
/> | ||
<label htmlFor="mobile-nav-button" className="mobile-menu"> | ||
<span /> | ||
<span /> | ||
<span /> | ||
</label> | ||
<nav | ||
className={classnames('dropdown-nav-bar', { | ||
'no-mobile': !mobileBreakpoint, | ||
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. What's the underlying concept that this represents? I think "no-mobile" doesn't necessarily communicate intent directly. Also, could you use the boolean that you calculated in line 70? |
||
})} | ||
aria-label={menuLabel} | ||
> | ||
{!!mobileBreakpoint && ( | ||
<React.Fragment> | ||
<input | ||
type="checkbox" | ||
id="mobile-nav-button" | ||
checked={isMobileMenuOpen} | ||
onChange={() => { | ||
setIsMobileMenuOpen(!isMobileMenuOpen) | ||
closeDesktopSubmenu() | ||
}} | ||
/> | ||
<label htmlFor="mobile-nav-button" className="mobile-menu"> | ||
<span /> | ||
<span /> | ||
<span /> | ||
</label> | ||
danparnella marked this conversation as resolved.
Show resolved
Hide resolved
|
||
</React.Fragment> | ||
)} | ||
<DropdownNavMenu | ||
openMenuIds={openMenuIds} | ||
toggleOpenMenuId={toggleOpenMenuId} | ||
closeDesktopSubmenu={closeDesktopSubmenu} | ||
baseUrl={baseUrl} | ||
menuItems={menuItems} | ||
menuLabel={menuLabel} | ||
hideDropdownButtonsBeforeFocus={hideDropdownButtonsBeforeFocus} | ||
hideMenuButtonsBeforeFocus={hideMenuButtonsBeforeFocus} | ||
/> | ||
</nav> | ||
) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -12,7 +12,7 @@ const propTypes = { | |
toggleOpenMenuId: PropTypes.func.isRequired, | ||
closeDesktopSubmenu: PropTypes.func.isRequired, | ||
menuLabel: PropTypes.string.isRequired, | ||
hideDropdownButtonsBeforeFocus: PropTypes.bool.isRequired, | ||
hideMenuButtonsBeforeFocus: PropTypes.bool.isRequired, | ||
} | ||
|
||
const defaultProps = {} | ||
|
@@ -24,7 +24,7 @@ function DropdownNavMenu({ | |
toggleOpenMenuId, | ||
closeDesktopSubmenu, | ||
menuLabel, | ||
hideDropdownButtonsBeforeFocus, | ||
hideMenuButtonsBeforeFocus, | ||
}) { | ||
return ( | ||
<ul className="dropdown-nav-menu" aria-label={menuLabel} role="menubar"> | ||
|
@@ -42,7 +42,7 @@ function DropdownNavMenu({ | |
closeDesktopSubmenu={closeDesktopSubmenu} | ||
toggleSubmenu={() => toggleOpenMenuId(id)} | ||
isFirstItem={isFirstParentItem} | ||
hideDropdownButtonBeforeFocus={hideDropdownButtonsBeforeFocus} | ||
hideMenuButtonBeforeFocus={hideMenuButtonsBeforeFocus} | ||
> | ||
{childItems && !isEmpty(childItems) && ( | ||
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. Do you need the additional |
||
<ul className="sub-menu" role="menu" aria-label={name}> | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -15,6 +15,7 @@ | |
*/ | ||
|
||
function isMobileView(mobileBreakpoint) { | ||
if (!mobileBreakpoint) return false | ||
// eslint-disable-next-line no-undef | ||
return window.top.innerWidth < mobileBreakpoint | ||
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. if |
||
} | ||
|
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.
Will this need to be ported over to the client template as well or will the styles automatically be applied when a user imports the component into their project?