Skip to content

fix(navExpand): remove defaultExpand in favor of just using isExpanded #1490

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
redallen merged 1 commit into from
Mar 5, 2019
Merged

fix(navExpand): remove defaultExpand in favor of just using isExpanded #1490

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 1 addition & 1 deletion packages/patternfly-4/react-core/src/components/Nav/NavExpandable.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,13 +3,13 @@ import { FunctionComponent, HTMLProps, ReactNode } from 'react';
export interface NavExpandableProps extends HTMLProps<HTMLDivElement> {
title: string;
srText?: string;
defaultExpanded?: boolean;
isExpanded?: boolean;
children?: ReactNode;
className?: string;
groupId?: string | number;
isActive?: boolean;
id?: string;
onExpand?(e: Event, val: boolean): void;
}

declare const NavExpandable: FunctionComponent<NavExpandableProps>;
Expand Down
55 changes: 31 additions & 24 deletions packages/patternfly-4/react-core/src/components/Nav/NavExpandable.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,6 @@ const propTypes = {
title: PropTypes.string.isRequired,
/** If defined, screen readers will read this text instead of the list title */
srText: PropTypes.string,
/** If true will default the list to be expanded */
defaultExpanded: PropTypes.bool,
/** Boolean to programatically expand or collapse section */
isExpanded: PropTypes.bool,
/** Anything that can be rendered inside of the expandable list */
Expand All @@ -28,45 +26,54 @@ const propTypes = {
/** Identifier to use for the section aria label */
id: PropTypes.string,
/** Additional props are spread to the container <li> */
'': PropTypes.any
'': PropTypes.any,
/** allow consumer to optionally override this callback and manage expand state externally */
onExpand: PropTypes.func
};

const defaultProps = {
srText: '',
defaultExpanded: false,
isExpanded: null,
isExpanded: false,
children: null,
className: '',
groupId: null,
isActive: false,
id: ''
id: '',
onExpand: undefined
};

class NavExpandable extends React.Component {
id = this.props.id || getUniqueId();
state = {
expandedState: false
};

componentDidMount() {
this.setState({ expandedState: this.props.isExpanded });
}

componentDidUpdate(prevProps) {
if (this.props.isExpanded !== prevProps.isExpanded) {
Copy link
Member

@priley86 priley86 Mar 5, 2019

Choose a reason for hiding this comment

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

should this be:

  componentDidUpdate() {
    if (this.props.isExpanded !== this.state.isExpanded) {

?

Copy link
Member

@priley86 priley86 Mar 5, 2019

Choose a reason for hiding this comment

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

sorry:

if (this.props.isExpanded !== prevProps.isExpanded || this.props.isExpanded !== this.state.expandedState)

Copy link
Member

@priley86 priley86 Mar 5, 2019

Choose a reason for hiding this comment

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

ok, made a few changes to address this. If the consumer is setting expandState externally, we should allow them to hook into this, otherwise they could be become out of sync. My changes here would allow the consumer to override the expand/collapse state at either the top level (Nav and the onToggle), or at a more granular level, the NavExpandable and the onExpand callback). They would then be able to hook into either (or neither if desired). See if this works on your side...
priley86@ee30e20

Copy link
Member

@priley86 priley86 Mar 5, 2019

Choose a reason for hiding this comment

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

downstream explanation/relevant changes...
openshift/console#1238 (comment)

this.setState({ expandedState: this.props.isExpanded });
}
}

onExpand = (e, val) => {
if (this.props.onExpand) {
this.props.onExpand(e, val);
} else {
this.setState({ expandedState: val });
}
};

render() {
const {
id,
title,
srText,
isExpanded,
defaultExpanded,
children,
className,
groupId,
isActive,
...props
} = this.props;
const { id, title, srText, isExpanded, children, className, groupId, isActive, ...props } = this.props;
const { expandedState } = this.state;

return (
<NavContext.Consumer>
{context => (
<NavToggle
defaultValue={defaultExpanded}
isExpanded={isExpanded}
groupId={groupId}
onToggle={context.onToggle}
>
<NavToggle groupId={groupId} onToggle={context.onToggle} onExpand={this.onExpand} isExpanded={expandedState}>
{({ toggleValue, toggle }) => (
<li
className={css(
Expand Down
29 changes: 10 additions & 19 deletions packages/patternfly-4/react-core/src/components/Nav/NavToggle.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,45 +3,36 @@ import PropTypes from 'prop-types';

const propTypes = {
children: PropTypes.func.isRequired,
defaultValue: PropTypes.bool,
isExpanded: PropTypes.bool,
groupId: PropTypes.oneOfType([PropTypes.string, PropTypes.number]),
onToggle: PropTypes.func
onToggle: PropTypes.func,
onExpand: PropTypes.func
};

const defaultProps = {
defaultValue: false,
isExpanded: false,
groupId: 0,
onToggle: () => undefined
onToggle: () => undefined,
onExpand: () => undefined
};

class NavToggle extends React.Component {
static propTypes = propTypes;
static defaultProps = defaultProps;
state = {
toggleValue: this.props.defaultValue
};
componentDidUpdate(prevProps) {
if (this.props.isExpanded !== prevProps.isExpanded) {
this.setState({ toggleValue: this.props.isExpanded });
}
}

handleToggle = e => {
// Item events can bubble up, ignore those
if (e.target.getAttribute('data-component') !== 'pf-nav-expandable') {
return;
}
const { toggleValue } = this.state;
const { groupId, onToggle } = this.props;
this.setState({
toggleValue: !toggleValue
});
onToggle(e, groupId, !toggleValue);
const { groupId, onToggle, onExpand, isExpanded } = this.props;
onToggle(e, groupId, !isExpanded);
onExpand(e, !isExpanded);
};

render() {
return this.props.children({
toggleValue: this.state.toggleValue,
toggleValue: this.props.isExpanded,
toggle: this.handleToggle
});
}
Expand Down
25 changes: 11 additions & 14 deletions packages/patternfly-4/react-core/src/components/Nav/__snapshots__/Nav.test.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -356,7 +356,7 @@ exports[`Expandable Nav List - Trigger toggle 1`] = `
overflow: hidden;
opacity: 0;
}
.pf-c-nav__item.expandable-group {
.pf-c-nav__item.pf-m-expanded.expandable-group {
display: block;
}
.pf-c-nav__list {
Expand Down Expand Up @@ -388,7 +388,6 @@ exports[`Expandable Nav List - Trigger toggle 1`] = `
>
<NavExpandable
className="expandable-group"
defaultExpanded={false}
groupId={null}
id="grp-1"
isActive={false}
Expand All @@ -397,17 +396,17 @@ exports[`Expandable Nav List - Trigger toggle 1`] = `
title="Section 1"
>
<NavToggle
defaultValue={false}
groupId={null}
isExpanded={true}
onExpand={[Function]}
onToggle={[Function]}
>
<li
className="pf-c-nav__item expandable-group"
className="pf-c-nav__item pf-m-expanded expandable-group"
onClick={[Function]}
>
<a
aria-expanded={false}
aria-expanded={true}
className="pf-c-nav__link"
data-component="pf-nav-expandable"
href="#"
Expand Down Expand Up @@ -450,7 +449,7 @@ exports[`Expandable Nav List - Trigger toggle 1`] = `
<section
aria-labelledby="grp-1"
className="pf-c-nav__subnav"
hidden={true}
hidden={null}
>
<ul
className="pf-c-nav__simple-list"
Expand Down Expand Up @@ -642,18 +641,17 @@ exports[`Expandable Nav List 1`] = `
>
<NavExpandable
className=""
defaultExpanded={false}
groupId={null}
id="grp-1"
isActive={false}
isExpanded={null}
isExpanded={false}
srText=""
title="Section 1"
>
<NavToggle
defaultValue={false}
groupId={null}
isExpanded={null}
isExpanded={false}
onExpand={[Function]}
onToggle={[Function]}
>
<li
Expand Down Expand Up @@ -904,18 +902,17 @@ exports[`Expandable Nav List with aria label 1`] = `
>
<NavExpandable
className=""
defaultExpanded={false}
groupId={null}
id="grp-1"
isActive={false}
isExpanded={null}
isExpanded={false}
srText="Section 1 - Example sub-navigation"
title="Section 1"
>
<NavToggle
defaultValue={false}
groupId={null}
isExpanded={null}
isExpanded={false}
onExpand={[Function]}
onToggle={[Function]}
>
<li
Expand Down
4 changes: 2 additions & 2 deletions packages/patternfly-4/react-core/src/components/Nav/examples/NavExpandableList.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ class NavExpandableList extends React.Component {
return (
<Nav onSelect={this.onSelect} onToggle={this.onToggle}>
<NavList>
<NavExpandable title="Link 1" groupId="grp-1" isActive={activeGroup === 'grp-1'} defaultExpanded>
<NavExpandable title="Link 1" groupId="grp-1" isActive={activeGroup === 'grp-1'} isExpanded>
<NavItem
preventDefault
to="#expandable-1"
Expand All @@ -52,7 +52,7 @@ class NavExpandableList extends React.Component {
Subnav Link 3
</NavItem>
</NavExpandable>
<NavExpandable title="Link 2" groupId="grp-2" isActive={activeGroup === 'grp-2'}>
<NavExpandable title="Link 2" groupId="grp-2" isActive={activeGroup === 'grp-2'} isExpanded>
<NavItem
preventDefault
onClick={this.handleItemOnclick}
Expand Down
8 changes: 1 addition & 7 deletions packages/patternfly-4/react-core/src/components/Nav/examples/NavExpandableTitlesList.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,13 +19,7 @@ class NavExpandableTitlesList extends React.Component {
return (
<Nav onSelect={this.onSelect}>
<NavList>
<NavExpandable
title="Link 1"
srText="SR Link"
groupId="grp-1"
isActive={activeGroup === 'grp-1'}
defaultExpanded
>
<NavExpandable title="Link 1" srText="SR Link" groupId="grp-1" isActive={activeGroup === 'grp-1'} isExpanded>
<NavItem
preventDefault
to="#sr-expandable-1"
Expand Down