Skip to content

Minor Nav Refactor + Fix for Overlapping Dropdown Menus #1160

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.

Sign up for GitHub

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

Merged
sghoweri merged 4 commits into from
Apr 26, 2020
Merged

Minor Nav Refactor + Fix for Overlapping Dropdown Menus #1160

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
d0155b5
refactor: split apart existing Nav UI into smaller, more manageable p…
bolt-bot Apr 25, 2020
409bef3
fix: only allow one top level nav item to be open at a time while ren…
bolt-bot Apr 25, 2020
6da41a1
fix: re-try Netlify preview to debug local vs prod rendering differences
bolt-bot Apr 25, 2020
7a8b418
fix: make sure the top-level Dropdown menus always open/close
bolt-bot Apr 25, 2020
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
218 changes: 30 additions & 188 deletions packages/uikit-workshop/src/scripts/components/pl-nav/pl-nav.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,161 +10,10 @@ import { BaseComponent } from '../base-component.js';
import Mousetrap from 'mousetrap';
import 'url-search-params-polyfill';

const SubSubList = props => {
const { children, category, elem } = props;
const reorderedChildren = [];

const nonViewAllItems = elem.noViewAll
? children.filter(item => item.patternName !== 'View All')
: children.filter(
item =>
item.patternName !== 'View All' && !item.patternName.includes(' Docs')
);
const viewAllItems = elem.noViewAll
? []
: children.filter(item => item.patternName === 'View All');

reorderedChildren.push(...viewAllItems, ...nonViewAllItems);

return (
<li className={`pl-c-nav__item pl-c-nav__item--${category.toLowerCase()}`}>
{viewAllItems.length > 0 ? (
viewAllItems.map(patternSubtypeItem => (
<div class="pl-c-nav__link--overview-wrapper">
<a
href={`patterns/${patternSubtypeItem.patternPath}`}
className={`pl-c-nav__link pl-c-nav__link--sublink
${
patternSubtypeItem.patternName === 'View All'
? 'pl-c-nav__link--overview pl-js-link-overview'
: 'pl-c-nav__link--subsublink'
}
`}
onClick={e =>
elem.handleClick(e, patternSubtypeItem.patternPartial)
}
data-patternpartial={patternSubtypeItem.patternPartial}
>
{patternSubtypeItem.patternName === 'View All'
? `${category}`
: patternSubtypeItem.patternName}
{patternSubtypeItem.patternState && (
<span
class={`pl-c-pattern-state pl-c-pattern-state--${patternSubtypeItem.patternState}`}
title={patternSubtypeItem.patternState}
/>
)}
</a>

{nonViewAllItems.length >= 1 && (
<SpecialButton
aria-controls={category}
onClick={elem.toggleSpecialNavPanel}
>
{category}
</SpecialButton>
)}
</div>
))
) : (
<Button aria-controls={category} onClick={elem.toggleNavPanel}>
{category}
</Button>
)}

{((viewAllItems.length && nonViewAllItems.length) ||
viewAllItems.length === 0) && (
<ol
id={category}
className={`pl-c-nav__subsublist pl-c-nav__subsublist--dropdown pl-js-acc-panel`}
>
{nonViewAllItems.map(patternSubtypeItem => (
<li class="pl-c-nav__item">
<a
href={`patterns/${patternSubtypeItem.patternPath}`}
className={`pl-c-nav__link pl-c-nav__link--sublink
${
patternSubtypeItem.patternName === 'View All'
? 'pl-c-nav__link--overview'
: 'pl-c-nav__link--subsublink'
}
`}
onClick={e =>
elem.handleClick(e, patternSubtypeItem.patternPartial)
}
data-patternpartial={patternSubtypeItem.patternPartial}
>
{patternSubtypeItem.patternName === 'View All'
? `${category} Overview`
: patternSubtypeItem.patternName}
{patternSubtypeItem.patternState && (
<span
class={`pl-c-pattern-state pl-c-pattern-state--${patternSubtypeItem.patternState}`}
title={patternSubtypeItem.patternState}
/>
)}
</a>
</li>
))}
</ol>
)}
</li>
);
};

const SpecialButton = props => {
return (
<button
className={`pl-c-nav__link pl-c-nav__link--section-dropdown pl-js-acc-handle`}
role="tab"
{...props}
>
{props.children}
<span
class="pl-c-nav__link-icon"
dangerouslySetInnerHTML={{
__html: '<pl-icon name="arrow-down"></pl-icon>',
}}
/>
</button>
);
};

const Button = props => {
return (
<button
className={`pl-c-nav__link pl-c-nav__link--dropdown pl-js-acc-handle`}
role="tab"
{...props}
>
<span className={`pl-c-nav__link-text`}>{props.children}</span>
<span
class="pl-c-nav__link-icon"
dangerouslySetInnerHTML={{
__html: '<pl-icon name="arrow-down"></pl-icon>',
}}
/>
</button>
);
};

const ButtonTitle = props => {
return (
<button
className={`pl-c-nav__link pl-c-nav__link--title pl-js-acc-handle`}
role="tab"
{...props}
>
<span
class="pl-c-nav__link-icon"
dangerouslySetInnerHTML={{
__html: '<pl-icon name="arrow-down"></pl-icon>',
}}
/>
<span className={`pl-c-nav__link-text`}>{props.children}</span>
</button>
);
};
import { NavTitle } from './src/NavTitle';
import { NavList } from './src/NavList';
import { NavLink } from './src/NavLink';
import { NavItem } from './src/NavItem';

@define
class Nav extends BaseComponent {
Expand Down Expand Up @@ -370,7 +219,22 @@ class Nav extends BaseComponent {

toggleNavPanel(e) {
const target = e.target;

target.classList.toggle('pl-is-active');

// when the Nav renders as a dropdown menu, only allow one top-level menu item to be open at a time to prevent overlap issues
if (this.layoutMode !== 'vertical' && window.innerWidth > 670) {
this.topLevelTriggers = document.querySelectorAll(
'.pl-c-nav__link--title.pl-is-active'
);

this.topLevelTriggers.forEach(trigger => {
if (trigger !== target) {
trigger.classList.remove('pl-is-active');
trigger.nextSibling.classList.remove('pl-is-active');
}
});
Comment on lines +231 to +236
Copy link
Contributor

@JosefBredereck JosefBredereck Apr 26, 2020

Choose a reason for hiding this comment

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

If you walk through all trigger, why you need to remove the pl-is-active class from the next sibling?

Copy link
Contributor Author

@sghoweri sghoweri Apr 26, 2020

Choose a reason for hiding this comment

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

This chunk of code was just copied over from what we were already doing in the full Nav cleanup method.

Keep in mind that despite changing how the Nav was previously being rendered, much of the current code here hasn't been fully refactored (hence there's still some cruft to clean up like what you're pointing out here).

}
}

rendered() {
Expand All @@ -393,32 +257,28 @@ class Nav extends BaseComponent {
return (
<ol class="pl-c-nav__list pl-js-pattern-nav-target">
{patternTypes.map((item, i) => {
const classes = classNames({
[`pl-c-nav__item pl-c-nav__item--${item.patternTypeLC}`]: true,
});

const patternItems = item.patternItems;

return (
<li className={classes}>
<ButtonTitle
<NavItem className={`pl-c-nav__item--${item.patternTypeLC}`}>
<NavTitle
aria-controls={item.patternTypeLC}
onClick={this.toggleNavPanel}
>
{item.patternTypeUC}
</ButtonTitle>
</NavTitle>
<ol
id={item.patternSubtypeUC}
className={`pl-c-nav__sublist pl-c-nav__sublist--dropdown pl-js-acc-panel`}
>
{item.patternTypeItems.map((patternSubtype, i) => {
return (
<SubSubList
<NavList
elem={this.elem}
category={patternSubtype.patternSubtypeUC}
>
{patternSubtype.patternSubtypeItems}
</SubSubList>
</NavList>
);
})}

Expand All @@ -428,31 +288,13 @@ class Nav extends BaseComponent {
patternItem.patternPartial.includes('viewall') ? (
''
) : (
<li class="pl-c-nav__item">
<a
href={`patterns/${patternItem.patternPath}`}
class="pl-c-nav__link pl-c-nav__link--pattern"
onClick={e =>
this.handleClick(e, patternItem.patternPartial)
}
data-patternpartial={patternItem.patternPartial}
tabindex="0"
>
{patternItem.patternName === 'View All'
? patternItem.patternName + ' ' + item.patternTypeUC
: patternItem.patternName}
{patternItem.patternState && (
<span
class={`pl-c-pattern-state pl-c-pattern-state--${patternItem.patternState}`}
title={patternItem.patternState}
/>
)}
</a>
</li>
<NavItem>
<NavLink item={patternItem} elem={this} />
</NavItem>
);
})}
</ol>
</li>
</NavItem>
);
})}

Expand All @@ -461,7 +303,7 @@ class Nav extends BaseComponent {
window.ishControls.ishControlsHide === undefined ||
(window.ishControls.ishControlsHide['views-all'] !== true &&
window.ishControls.ishControlsHide.all !== true)) && (
<li class="pl-c-nav__item">
<NavItem>
<a
onClick={e => this.handleClick(e, 'all')}
href="styleguide/html/styleguide.html"
Expand All @@ -471,7 +313,7 @@ class Nav extends BaseComponent {
>
All
</a>
</li>
</NavItem>
)}
</ol>
);
Expand Down
19 changes: 19 additions & 0 deletions packages/uikit-workshop/src/scripts/components/pl-nav/src/NavButton.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
import { h } from 'preact';

export const NavButton = props => {
return (
<button
className={`pl-c-nav__link pl-c-nav__link--dropdown pl-js-acc-handle`}
role="tab"
{...props}
>
<span className={`pl-c-nav__link-text`}>{props.children}</span>
<span
class="pl-c-nav__link-icon"
dangerouslySetInnerHTML={{
__html: '<pl-icon name="arrow-down"></pl-icon>',
}}
/>
</button>
);
};
8 changes: 8 additions & 0 deletions packages/uikit-workshop/src/scripts/components/pl-nav/src/NavItem.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
import { h } from 'preact';
import cx from 'classnames';

export const NavItem = props => {
const classes = cx('pl-c-nav__item', props.className);

return <li className={classes}>{props.children}</li>;
};
26 changes: 26 additions & 0 deletions packages/uikit-workshop/src/scripts/components/pl-nav/src/NavLink.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
import { h } from 'preact';
import { PatternState } from './PatternState';

export const NavLink = props => {
return (
<a
href={`patterns/${props.item.patternPath}`}
className={`pl-c-nav__link pl-c-nav__link--sublink
${
props.item.patternName === 'View All'
? 'pl-c-nav__link--overview'
: 'pl-c-nav__link--subsublink'
}
`}
onClick={e => props.elem.handleClick(e, props.item.patternPartial)}
data-patternpartial={props.item.patternPartial}
>
{props.item.patternName === 'View All' && props.category
? `${props.category}`
: props.item.patternName}
{props.item.patternState && (
<PatternState variant={props.item.patternState} />
)}
</a>
);
};
Loading