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.

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

Jump to bottom

fix(sidenav): sidenavmenu stays open when in rail #3626

Merged
merged 13 commits into from
Aug 22, 2019
Merged

fix(sidenav): sidenavmenu stays open when in rail #3626

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
b0866e0
fix(sidenav): sidenavmenu stays open when in rail
Aug 1, 2019
b64d688
Merge branch 'master' into 3527-submenu-not-collapsing-in-rails
tw15egan Aug 1, 2019
7d8788f
fix(sidenav): sidenavmenu stays open when in rail
Aug 2, 2019
d67f853
Merge branch 'master' into 3527-submenu-not-collapsing-in-rails
tw15egan Aug 2, 2019
0a3293a
fix(sidenav): sidenavmenu stays open when in rail
Aug 2, 2019
62c79d9
fix(sidenav): sidenavmenu stays open when in rail
Aug 14, 2019
1688fee
fix(sidenav): sidenavmenu stays open when in rail
Aug 14, 2019
0d177f7
fix(sidenav): make sidenav open/close via keyboard
Aug 19, 2019
0a034e8
fix(sidenav): make sidenav open/close via keyboard
Aug 19, 2019
ee67d78
fix(sidenav): sidenavmenu stays open when in rail
Aug 21, 2019
136970f
Merge branch 'master' into 3527-submenu-not-collapsing-in-rails
matthew-chirgwin Aug 21, 2019
340a641
Merge branch 'master' into 3527-submenu-not-collapsing-in-rails
tw15egan Aug 21, 2019
94d6a0d
Merge branch 'master' into 3527-submenu-not-collapsing-in-rails
matthew-chirgwin Aug 22, 2019
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
6 changes: 6 additions & 0 deletions packages/react/src/components/UIShell/Link.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,12 @@ const LinkPropTypes = {
* alternative tag names or custom components like `Link` from `react-router`.
*/
element: PropTypes.elementType,

/**
* Property to indicate if the side nav container is open (or not). Use to
* keep local state and styling in step with the SideNav expansion state.
*/
isSideNavExpanded: PropTypes.bool,
};

Link.displayName = 'Link';
Expand Down
29 changes: 25 additions & 4 deletions packages/react/src/components/UIShell/SideNav.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,9 @@ const SideNav = React.forwardRef(function SideNav(props, ref) {

const { current: controlled } = useRef(expandedProp !== undefined);
const [expandedState, setExpandedState] = useState(defaultExpanded);
const [expandedViaHoverState, setExpandedViaHoverState] = useState(
defaultExpanded
);
const expanded = controlled ? expandedProp : expandedState;
const handleToggle = (event, value = !expanded) => {
if (!controlled) {
Expand All @@ -42,6 +45,9 @@ const SideNav = React.forwardRef(function SideNav(props, ref) {
if (onToggle) {
onToggle(event, value);
}
if (controlled || isRail) {
setExpandedViaHoverState(value);
}
};

const accessibilityLabel = {
Expand All @@ -56,7 +62,7 @@ const SideNav = React.forwardRef(function SideNav(props, ref) {

const className = cx({
[`${prefix}--side-nav`]: true,
[`${prefix}--side-nav--expanded`]: expanded,
[`${prefix}--side-nav--expanded`]: expanded || expandedViaHoverState,
[`${prefix}--side-nav--collapsed`]: !expanded && isFixedNav,
[`${prefix}--side-nav--rail`]: isRail,
[customClassName]: !!customClassName,
Expand All @@ -69,6 +75,21 @@ const SideNav = React.forwardRef(function SideNav(props, ref) {
[`${prefix}--side-nav__overlay-active`]: expanded,
});

let childrenToRender = children;

// if a rail, pass the expansion state as a prop, so children can update themselves to match
if (isRail) {
childrenToRender = React.Children.map(children, child => {
// if we are controlled, check for if we have hovered over or the expanded state, else just use the expanded state (uncontrolled)
let currentExpansionState = controlled
? expandedViaHoverState || expanded
: expanded;
return React.cloneElement(child, {
isSideNavExpanded: currentExpansionState,
});
});
}

return (
<>
{isFixedNav ? null : <div className={overlayClassName} />}
Expand All @@ -78,9 +99,9 @@ const SideNav = React.forwardRef(function SideNav(props, ref) {
{...accessibilityLabel}
onFocus={event => handleToggle(event, true)}
onBlur={event => handleToggle(event, false)}
onMouseEnter={() => handleToggle(true)}
onMouseLeave={() => handleToggle(false)}>
{children}
onMouseEnter={() => handleToggle(true, true)}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
onMouseEnter={() => handleToggle(true, true)}
onMouseEnter={event => handleToggle(event, true)}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@asudoh - before my changes, handleToggle was called here with just true as the only argument. I can change it to whatever the event is as you suggest, but if someone downstream passes the onToggle property to the SideNav and expects the bool value, would that not be a breaking change for them?

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you for putting your thoughts on this and elaborating it @matthew-chirgwin! I now see your point (I missed it, tricked by argument name), and I now think the best is keeping handleToggle() call intact - Meaning, call with one arg.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @asudoh - the only issue with moving to one argument is the effect that would then have on how I changed handleToggle to handle this case (lines 48-50). That change could be pushed into its own function, but would that add any value?

Copy link
Contributor

Choose a reason for hiding this comment

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

Just leaving a note, though this is merged already; I think the best approach is making handleToggle() obvious that the first arg is optional and and the second arg becomes the first arg when the caller doesn't specify the first arg.

onMouseLeave={() => handleToggle(false, false)}>
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
onMouseLeave={() => handleToggle(false, false)}>
onMouseLeave={event => handleToggle(event, false)}>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@asudoh - same here as my comment above

{childrenToRender}
</nav>
</>
);
Expand Down
6 changes: 6 additions & 0 deletions packages/react/src/components/UIShell/SideNavFooter.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,12 @@ SideNavFooter.propTypes = {
* with. Useful for controlling the expansion state of the side navigation.
*/
onToggle: PropTypes.func.isRequired,

/**
* Property to indicate if the side nav container is open (or not). Use to
* keep local state and styling in step with the SideNav expansion state.
*/
isSideNavExpanded: PropTypes.bool,
};

SideNavFooter.defaultProps = {
Expand Down
6 changes: 6 additions & 0 deletions packages/react/src/components/UIShell/SideNavHeader.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,12 @@ SideNavHeader.propTypes = {
*/
renderIcon: PropTypes.oneOfType([PropTypes.func, PropTypes.object])
.isRequired,

/**
* Property to indicate if the side nav container is open (or not). Use to
* keep local state and styling in step with the SideNav expansion state.
*/
isSideNavExpanded: PropTypes.bool,
};

export default SideNavHeader;
17 changes: 15 additions & 2 deletions packages/react/src/components/UIShell/SideNavItems.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,16 @@ import React from 'react';

const { prefix } = settings;

const SideNavItems = ({ className: customClassName, children }) => {
const SideNavItems = ({
className: customClassName,
children,
isSideNavExpanded,
}) => {
const className = cx([`${prefix}--side-nav__items`], customClassName);
return <ul className={className}>{children}</ul>;
const childrenWithExpandedState = React.Children.map(children, child => {
return React.cloneElement(child, { isSideNavExpanded });
});
return <ul className={className}>{childrenWithExpandedState}</ul>;
};

SideNavItems.propTypes = {
Expand All @@ -28,6 +35,12 @@ SideNavItems.propTypes = {
* container
*/
children: PropTypes.node.isRequired,

/**
* Property to indicate if the side nav container is open (or not). Use to
* keep local state and styling in step with the SideNav expansion state.
*/
isSideNavExpanded: PropTypes.bool,
};

export default SideNavItems;
6 changes: 6 additions & 0 deletions packages/react/src/components/UIShell/SideNavLink.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,12 @@ SideNavLink.propTypes = {
* Specify the text content for the link
*/
children: PropTypes.string.isRequired,

/**
* Property to indicate if the side nav container is open (or not). Use to
* keep local state and styling in step with the SideNav expansion state.
*/
isSideNavExpanded: PropTypes.bool,
};

SideNavLink.defaultProps = {
Expand Down
49 changes: 42 additions & 7 deletions packages/react/src/components/UIShell/SideNavMenu.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,17 +48,45 @@ export class SideNavMenu extends React.Component {
* be closed.
*/
defaultExpanded: PropTypes.bool,

/**
* Property to indicate if the side nav container is open (or not). Use to
* keep local state and styling in step with the SideNav expansion state.
*/
isSideNavExpanded: PropTypes.bool,
};

static defaultProps = {
defaultExpanded: false,
isActive: false,
};

static getDerivedStateFromProps = (props, state) => {
let derivedState = null;

if (props.isSideNavExpanded === false && state.isExpanded === true) {
derivedState = {
isExpanded: props.isSideNavExpanded,
wasPreviouslyExpanded: true,
};
} else if (
props.isSideNavExpanded === true &&
state.wasPreviouslyExpanded === true
) {
derivedState = {
isExpanded: props.isSideNavExpanded,
wasPreviouslyExpanded: false,
};
}

return derivedState;
};

constructor(props) {
super(props);
this.state = {
isExpanded: props.defaultExpanded || false,
wasPreviouslyExpanded: props.defaultExpanded || false,
};
}

Expand All @@ -78,13 +106,20 @@ export class SideNavMenu extends React.Component {
const { isExpanded } = this.state;

let hasActiveChild;
if (children && typeof children === Object) {
hasActiveChild = children.some(child => {
if (child.props.isActive === true || child.props['aria-current']) {
return true;
}
return false;
});
if (children) {
// if we have children, either a single or multiple, find if it is active
hasActiveChild = Array.isArray(children)
? children.some(child => {
if (
child.props &&
(child.props.isActive === true || child.props['aria-current'])
) {
return true;
}
return false;
})
: children.props &&
(children.props.isActive === true || children.props['aria-current']);
}

const className = cx({
Expand Down
138 changes: 138 additions & 0 deletions packages/react/src/components/UIShell/UIShell-story.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -637,4 +637,142 @@ storiesOf('UI Shell', module)
<StoryContent />
</>
))
)
.add(
'SideNav Rail',
withReadme(readme, () => (
<>
<SideNav aria-label="Side navigation" isRail>
<SideNavItems>
<SideNavMenu renderIcon={Fade16} title="Category title">
<SideNavMenuItem href="javascript:void(0)">Link</SideNavMenuItem>
<SideNavMenuItem aria-current="page" href="javascript:void(0)">
Link
</SideNavMenuItem>
<SideNavMenuItem href="javascript:void(0)">Link</SideNavMenuItem>
</SideNavMenu>
<SideNavMenu renderIcon={Fade16} title="Category title">
<SideNavMenuItem href="javascript:void(0)">Link</SideNavMenuItem>
<SideNavMenuItem href="javascript:void(0)">Link</SideNavMenuItem>
<SideNavMenuItem href="javascript:void(0)">Link</SideNavMenuItem>
</SideNavMenu>
<SideNavMenu renderIcon={Fade16} title="Category title">
<SideNavMenuItem href="javascript:void(0)">Link</SideNavMenuItem>
<SideNavMenuItem href="javascript:void(0)">Link</SideNavMenuItem>
<SideNavMenuItem href="javascript:void(0)">Link</SideNavMenuItem>
</SideNavMenu>
<SideNavLink renderIcon={Fade16} href="javascript:void(0)">
Link
</SideNavLink>
<SideNavLink renderIcon={Fade16} href="javascript:void(0)">
Link
</SideNavLink>
</SideNavItems>
</SideNav>
<StoryContent />
</>
))
)
.add(
'SideNav Rail w/Header',
withReadme(readme, () => (
<HeaderContainer
render={({ isSideNavExpanded, onClickSideNavExpand }) => (
<>
<Header aria-label="IBM Platform Name">
<SkipToContent />
<HeaderMenuButton
aria-label="Open menu"
isCollapsible
onClick={onClickSideNavExpand}
isActive={isSideNavExpanded}
/>
<HeaderName href="#" prefix="IBM">
[Platform]
</HeaderName>
<HeaderNavigation aria-label="IBM [Platform]">
<HeaderMenuItem href="#">Link 1</HeaderMenuItem>
<HeaderMenuItem href="#">Link 2</HeaderMenuItem>
<HeaderMenuItem href="#">Link 3</HeaderMenuItem>
<HeaderMenu aria-label="Link 4">
<HeaderMenuItem href="#">Sub-link 1</HeaderMenuItem>
<HeaderMenuItem href="#">Sub-link 2</HeaderMenuItem>
<HeaderMenuItem href="#">Sub-link 3</HeaderMenuItem>
</HeaderMenu>
</HeaderNavigation>
<HeaderGlobalBar>
<HeaderGlobalAction
aria-label="Search"
onClick={action('search click')}>
<Search20 />
</HeaderGlobalAction>
<HeaderGlobalAction
aria-label="Notifications"
onClick={action('notification click')}>
<Notification20 />
</HeaderGlobalAction>
<HeaderGlobalAction
aria-label="App Switcher"
onClick={action('app-switcher click')}>
<AppSwitcher20 />
</HeaderGlobalAction>
</HeaderGlobalBar>
<SideNav
aria-label="Side navigation"
isRail
expanded={isSideNavExpanded}>
<SideNavItems>
<SideNavMenu renderIcon={Fade16} title="Category title">
<SideNavMenuItem href="javascript:void(0)">
Link
</SideNavMenuItem>
<SideNavMenuItem
aria-current="page"
href="javascript:void(0)">
Link
</SideNavMenuItem>
<SideNavMenuItem href="javascript:void(0)">
Link
</SideNavMenuItem>
</SideNavMenu>
<SideNavMenu renderIcon={Fade16} title="Category title">
<SideNavMenuItem href="javascript:void(0)">
Link
</SideNavMenuItem>
<SideNavMenuItem
aria-current="page"
href="javascript:void(0)">
Link
</SideNavMenuItem>
<SideNavMenuItem href="javascript:void(0)">
Link
</SideNavMenuItem>
</SideNavMenu>
<SideNavMenu renderIcon={Fade16} title="Category title">
<SideNavMenuItem href="javascript:void(0)">
Link
</SideNavMenuItem>
<SideNavMenuItem
aria-current="page"
href="javascript:void(0)">
Link
</SideNavMenuItem>
<SideNavMenuItem href="javascript:void(0)">
Link
</SideNavMenuItem>
</SideNavMenu>
<SideNavLink renderIcon={Fade16} href="javascript:void(0)">
Link
</SideNavLink>
<SideNavLink renderIcon={Fade16} href="javascript:void(0)">
Link
</SideNavLink>
</SideNavItems>
</SideNav>
</Header>
<StoryContent />
</>
)}
/>
))
);
Loading