Skip to content

fix(Forms;SideNav;TreeView): Make React.Children null safe #535

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
jbadan merged 4 commits into from
May 9, 2019
Merged

fix(Forms;SideNav;TreeView): Make React.Children null safe #535

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
396815f
feat(Forms;SideNav;TreeView): Make React.Children null safe
skvale May 3, 2019
f75c7d3
test(treeview): Minimize snapshot size
skvale May 3, 2019
cfd8a9b
remove an extra comment
skvale May 3, 2019
ad1b676
test(TreeRow): Move tests after rebasing on master
skvale May 8, 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
18 changes: 7 additions & 11 deletions src/Forms/FormRadioGroup.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,17 +15,13 @@ class FormRadioGroup extends Component {
return (
<div
{...props}>
{React.Children.map(children, child => {
if (React.isValidElement(child)) {
return React.cloneElement(child, {
disabled: child.props.disabled || disabled,
inline: child.props.inline || inline,
name: child.props.name || this.groupId,
onChange: child.props.onChange || onChange
});
} else {
return child;
}
{React.Children.toArray(children).map(child => {
return React.cloneElement(child, {
disabled: child.props.disabled || disabled,
inline: child.props.inline || inline,
name: child.props.name || this.groupId,
onChange: child.props.onChange || onChange
});
})}
</div>
);
Expand Down
14 changes: 5 additions & 9 deletions src/SideNavigation/SideNav.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,15 +40,11 @@ class SideNav extends Component {

return (
<nav {...rest} className={sideNavClasses}>
{React.Children.map(children, (child) => {
if (React.isValidElement(child)) {
return React.cloneElement(child, {
onItemSelect: this.handleSelect,
selectedId: this.state.selectedId
});
} else {
return child;
}
{React.Children.toArray(children).map(child => {
return React.cloneElement(child, {
onItemSelect: this.handleSelect,
selectedId: this.state.selectedId
});
})}
</nav>
);
Expand Down
18 changes: 7 additions & 11 deletions src/SideNavigation/_SideNavList.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,17 +30,13 @@ class SideNavList extends React.Component {
aria-expanded={hasParent && open}
aria-hidden={hasParent && !open}
className={sideNavListClasses}>
{React.Children.map(children, (child) => {
if (React.isValidElement(child)) {
return React.cloneElement(child, {
isSubItem: hasParent,
onItemSelect: onItemSelect,
selected: selectedId === child.props.id,
selectedId: selectedId
});
} else {
return child;
}
{React.Children.toArray(children).map(child => {
return React.cloneElement(child, {
isSubItem: hasParent,
onItemSelect: onItemSelect,
selected: selectedId === child.props.id,
selectedId: selectedId
});
})}
</ul>
);
Expand Down
13 changes: 5 additions & 8 deletions src/SideNavigation/_SideNavListItem.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,11 +38,8 @@ class SideNavListItem extends React.Component {
);
};

let hasChild = false;
React.Children.forEach(children, (child) => {
if (React.isValidElement(child) && child.type === SideNavList) {
hasChild = true;
}
const hasChild = React.Children.toArray(children).some(child => {
return child.type === SideNavList;
});

const renderLink = () => {
Expand Down Expand Up @@ -72,8 +69,8 @@ class SideNavListItem extends React.Component {
className='fd-side-nav__item'
key={id}>
{url && renderLink()}
{React.Children.map(children, (child) => {
if (React.isValidElement(child) && child.type !== SideNavList) {
{React.Children.toArray(children).map(child => {
if (child.type !== SideNavList) {
return React.cloneElement(child, {
children: (<React.Fragment>
{glyph ? (
Expand All @@ -92,7 +89,7 @@ class SideNavListItem extends React.Component {
}
}
});
} else if (React.isValidElement(child) && child.type === SideNavList) {
} else if (child.type === SideNavList) {
return React.cloneElement(child, {
hasParent: true,
onItemSelect: onItemSelect,
Expand Down
41 changes: 41 additions & 0 deletions src/TreeView/TreeRow.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -183,6 +183,42 @@ describe('<TreeView.Row />', () => {
</TreeView>
);

const falseyCondition = false;
const conditionalTreeView = (
<TreeView>
<TreeView.Head>
<TreeView.Col>Column Header 1</TreeView.Col>
{falseyCondition && <TreeView.Col>Column Header 2</TreeView.Col>}
<TreeView.Col>Column Header 3</TreeView.Col>
<TreeView.Col>Column Header 4</TreeView.Col>
</TreeView.Head>
<TreeView.Tree>
<TreeView.Item>
<TreeView.Row>
<TreeView.Col>First Level</TreeView.Col>
</TreeView.Row>
<TreeView.Branch>
<TreeView.Item>
<TreeView.Row>
{falseyCondition && (
<TreeView.Col>
<a href='http://me.com'>First Level</a>
</TreeView.Col>
)}
<TreeView.Col>Second level</TreeView.Col>
<TreeView.Col />
<TreeView.Col />
</TreeView.Row>
{falseyCondition && <TreeView.Branch />}
</TreeView.Item>
{falseyCondition && <TreeView.Item />}
</TreeView.Branch>
</TreeView.Item>
{falseyCondition && <TreeView.Item />}
</TreeView.Tree>
</TreeView>
);

test('create tree component', () => {
// multi-level tree
let component = renderer.create(multiLevelTreeView);
Expand All @@ -193,6 +229,11 @@ describe('<TreeView.Row />', () => {
component = renderer.create(richTreeView);
tree = component.toJSON();
expect(tree).toMatchSnapshot();

// conditional tree
component = renderer.create(conditionalTreeView);
tree = component.toJSON();
expect(tree).toMatchSnapshot();
});

test('open all tree from header', () => {
Expand Down
46 changes: 22 additions & 24 deletions src/TreeView/TreeView.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,30 +117,28 @@ class TreeView extends Component {

return (
<div {...rest}>
{
React.Children.map(children, (child) => {
const isTreeHead = child.type && child.type.displayName === 'TreeView.Head';
const isTree = child.type && child.type.displayName === 'TreeView.Tree';

if (isTreeHead) {
// Pass expand all callbacks to TreeHead
return React.cloneElement(child, {
onExpandAll: this.toggleExpandAll,
isExpanded: isExpandAll
});
}

if (isTree) {
// Pass expand callbacks to Tree
return React.cloneElement(child, {
expandData,
onExpandClick: this.toggleExpand
});
}

return child;
})
}
{React.Children.toArray(children).map(child => {
const isTreeHead = child.type && child.type.displayName === 'TreeView.Head';
const isTree = child.type && child.type.displayName === 'TreeView.Tree';

if (isTreeHead) {
// Pass expand all callbacks to TreeHead
return React.cloneElement(child, {
onExpandAll: this.toggleExpandAll,
isExpanded: isExpandAll
});
}

if (isTree) {
// Pass expand callbacks to Tree
return React.cloneElement(child, {
expandData,
onExpandClick: this.toggleExpand
});
}

return child;
})}
</div>
);
}
Expand Down
16 changes: 7 additions & 9 deletions src/TreeView/_BaseTree.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,15 +26,13 @@ class BaseTree extends Component {
aria-hidden={level > 0 && !isExpanded}
className={className}
role={level === 0 ? 'tree' : 'group'}>
{
React.Children.map(children, (child) => {
return React.cloneElement(child, {
expandData,
level,
onExpandClick
});
})
}
{React.Children.toArray(children).map(child => {
return React.cloneElement(child, {
expandData,
level,
onExpandClick
});
})}
</ul>
);
}
Expand Down
2 changes: 1 addition & 1 deletion src/TreeView/_TreeHead.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ class TreeHead extends Component {
<div {...rest} className={headerClassName}>
<div className='fd-tree__row fd-tree__row--header'>
{
React.Children.map(children, (child, index) => {
React.Children.toArray(children).map((child, index) => {
const isFirstTreeCol = index === 0 && child.type && child.type.displayName === 'TreeView.Col';

// Add control class to first TreeCol element
Expand Down
28 changes: 12 additions & 16 deletions src/TreeView/_TreeItem.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,33 +31,29 @@ class TreeItem extends Component {
const isExpanded = isExpandedProp || !!expandData[this.rowId];

// Render child TreeBranch with correct props
const childBranch = React.Children.map(children, (child) => {
const isTreeBranch = child.type && child.type.displayName === 'TreeView.Branch';

return isTreeBranch ?
React.cloneElement(child, {
const childBranch = React.Children.toArray(children)
.filter(child => child.type && child.type.displayName === 'TreeView.Branch')
.map(child => {
return React.cloneElement(child, {
expandData,
onExpandClick,
isExpanded,
// Increment child branch level
level: level + 1
}) :
null;
});
});
});

// Render child TreeRow with correct props
const childRow = React.Children.map(children, (child) => {
const isTreeRow = child.type && child.type.displayName === 'TreeView.Row';

return isTreeRow ?
React.cloneElement(child, {
const childRow = React.Children.toArray(children)
.filter(child => child.type && child.type.displayName === 'TreeView.Row')
.map(child => {
return React.cloneElement(child, {
isExpanded,
onExpandClick: () => onExpandClick(this.rowId),
isParent: !!childBranch[0],
rowId: this.rowId
}) :
null;
});
});
});

return (
<li
Expand Down
2 changes: 1 addition & 1 deletion src/TreeView/_TreeRow.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ class TreeRow extends Component {
} = this.props;

// Render child TreeCols
const cells = React.Children.map(children, (child, index) => {
const cells = React.Children.toArray(children).map((child, index) => {
const isTreeCol = child.type && child.type.displayName === 'TreeView.Col';
const isFirstTreeCol = index === 0 && isTreeCol;

Expand Down
95 changes: 95 additions & 0 deletions src/TreeView/__snapshots__/TreeRow.test.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -722,3 +722,98 @@ exports[`<TreeView.Row /> create tree component 2`] = `
</ul>
</div>
`;

exports[`<TreeView.Row /> create tree component 3`] = `
<div>
<div
className="fd-tree fd-tree--header"
>
<div
className="fd-tree__row fd-tree__row--header"
>
<div
className="fd-tree__col fd-tree__col--control"
>
<div>
<button
aria-label="expand all"
aria-pressed={false}
className="fd-tree__control"
onClick={[Function]}
/>
Column Header 1
</div>
</div>
<div
className="fd-tree__col"
>
Column Header 3
</div>
<div
className="fd-tree__col"
>
Column Header 4
</div>
</div>
</div>
<ul
aria-hidden={false}
className="fd-tree"
role="tree"
>
<li
aria-expanded={false}
className="fd-tree__item"
id="fd-18"
role="treeitem"
>
<div
className="fd-tree__row"
>
<div
className="fd-tree__col fd-tree__col--control"
>
<div>
<button
aria-controls="fd-18"
aria-label="expand"
aria-pressed={false}
className="fd-tree__control"
onClick={[Function]}
/>
First Level
</div>
</div>
</div>
<ul
aria-hidden={true}
className="fd-tree__group fd-tree__group--sublevel-1 is-hidden"
role="group"
>
<li
aria-expanded={false}
className="fd-tree__item"
id="fd-19"
role="treeitem"
>
<div
className="fd-tree__row"
>
<div
className="fd-tree__col fd-tree__col--control"
>
Second level
</div>
<div
className="fd-tree__col"
/>
<div
className="fd-tree__col"
/>
</div>
</li>
</ul>
</li>
</ul>
</div>
`;