-
Notifications
You must be signed in to change notification settings - Fork 114
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
Polish side navigation styling #155
Changes from all commits
20df861
63cfc53
a1292fd
6d6f6d3
ce310a1
0f0e556
9075bdf
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 |
---|---|---|
@@ -0,0 +1,3 @@ | ||
import React from 'react'; | ||
|
||
export const BreadcrumbContext = React.createContext([]); | ||
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,48 +1,65 @@ | ||
import React from 'react'; | ||
import React, { useState, useContext } from 'react'; | ||
import PropTypes from 'prop-types'; | ||
import { Link } from 'gatsby'; | ||
import cx from 'classnames'; | ||
import { BreadcrumbContext } from './BreadcrumbContext'; | ||
|
||
import { link } from '../types'; | ||
import styles from './Sidebar.module.scss'; | ||
|
||
// recursively create navigation | ||
const renderNav = (page, index) => ( | ||
<li key={index}> | ||
{page.url ? ( | ||
<Link to={page.url} className={cx({ [styles.isActive]: page.active })}> | ||
{page.displayName} | ||
</Link> | ||
const renderNav = (pages, depthLevel = 0) => { | ||
return pages.map((page, index) => { | ||
const crumbs = useContext(BreadcrumbContext).flatMap((x) => x.displayName); | ||
const [isDisplay, setIsDisplay] = useState( | ||
crumbs.length === depthLevel || crumbs.includes(page.displayName) | ||
); | ||
const isCurrentPage = crumbs[crumbs.length - 1] === page.displayName; | ||
|
||
const display = page.url ? ( | ||
<Link to={page.url}>{page.displayName}</Link> | ||
) : ( | ||
<div>{page.displayName}</div> | ||
)} | ||
{page.children && <ul>{page.children.map(renderNav)}</ul>} | ||
</li> | ||
); | ||
<div | ||
role="button" | ||
onClick={() => setIsDisplay(!isDisplay)} | ||
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. Out of curiosity, if you do have a section that is collapsed and can be expanded because it doesn't include a |
||
onKeyPress={() => setIsDisplay(!isDisplay)} | ||
tabIndex={0} | ||
> | ||
{page.displayName} | ||
</div> | ||
); | ||
let subNav; | ||
|
||
const Sidebar = ({ className, pages, isOpen, toggle }) => ( | ||
if (page.children) { | ||
subNav = renderNav(page.children, depthLevel + 1); | ||
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. [nit] Since this <ul {/* ... */}>
{page.children && renderNav(page.children, depthLevel + 1)}
<ul> Totally your call on what you find more readable. |
||
} | ||
return ( | ||
<li | ||
className={cx(styles[`navDepth${depthLevel}`], { | ||
[styles.isCurrentPage]: isCurrentPage, | ||
})} | ||
key={index} | ||
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. Since the URLs should all be unique, I think it would be better to use that |
||
> | ||
{display} | ||
<ul className={cx(styles.nestedNav, { [styles.isDisplay]: isDisplay })}> | ||
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 are no children for the page, does this |
||
{subNav} | ||
</ul> | ||
</li> | ||
); | ||
}); | ||
}; | ||
|
||
const Sidebar = ({ className, pages, isOpen }) => ( | ||
<aside className={cx(styles.sidebar, className, { [styles.isOpen]: isOpen })}> | ||
<Link to="/" className={styles.logo} /> | ||
<div className={styles.top}> | ||
<h3>Pages</h3> | ||
<button | ||
aria-expanded={isOpen} | ||
aria-label="Main Menu Toggle" | ||
type="button" | ||
onClick={() => toggle()} | ||
> | ||
{isOpen ? 'close' : 'open'} | ||
</button> | ||
</div> | ||
<nav role="navigation" aria-label="Sidebar"> | ||
<ul>{pages.map(renderNav)}</ul> | ||
<ul className={styles.listNav}>{renderNav(pages)}</ul> | ||
</nav> | ||
</aside> | ||
); | ||
|
||
Sidebar.propTypes = { | ||
className: PropTypes.string, | ||
toggle: PropTypes.func.isRequired, | ||
pages: PropTypes.arrayOf(link).isRequired, | ||
isOpen: PropTypes.bool, | ||
}; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,6 @@ | ||
.sidebar { | ||
font-size: 0.875rem; | ||
|
||
ul { | ||
margin: 0; | ||
padding-left: 1rem; | ||
|
@@ -9,15 +11,6 @@ | |
text-decoration: none; | ||
color: var(--color-black); | ||
display: inline-block; | ||
padding: 0.2rem 0; | ||
|
||
&:hover { | ||
text-decoration: underline; | ||
} | ||
|
||
&.isActive { | ||
font-weight: bold; | ||
} | ||
} | ||
|
||
h3 { | ||
|
@@ -67,3 +60,52 @@ | |
margin-right: 1rem; | ||
background-image: url('../images/developers-logo.svg'); | ||
} | ||
|
||
.nestedNav { | ||
display: none; | ||
} | ||
|
||
.isDisplay { | ||
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. Would it be better to call this class name |
||
display: block; | ||
} | ||
|
||
.listNav { | ||
ul { | ||
padding-left: 1rem; | ||
} | ||
} | ||
|
||
.navDepth0 { | ||
color: var(--color-black); | ||
font-weight: bold; | ||
div { | ||
margin: 1rem 0; | ||
} | ||
li { | ||
margin: 1rem 0; | ||
} | ||
} | ||
|
||
.navDepth1 { | ||
font-weight: normal; | ||
ul { | ||
padding-left: 0.2rem; | ||
} | ||
} | ||
|
||
.navDepth2 { | ||
font-weight: bold; | ||
text-transform: uppercase; | ||
color: var(--color-neutrals-600); | ||
font-size: 14px; | ||
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. [style] Prefer 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 see that the |
||
} | ||
|
||
.navDepth3 { | ||
font-weight: normal; | ||
text-transform: initial; | ||
color: var(--color-black); | ||
} | ||
|
||
.isCurrentPage { | ||
font-weight: bold; | ||
} |
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.
[nit] You can make this the default export since this is the "main" thing exported from this file.