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

Reduce complexity on Dropdown component #387

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Reduce complexity on Dropdown component #387

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d624cac
Use context to avoid logic to clone elements
Apr 20, 2022
08fbac0
update storybook (Was causing issues)
Apr 20, 2022
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
32,945 changes: 6,349 additions & 26,596 deletions package-lock.json
View file
Open in desktop

Large diffs are not rendered by default.

24 changes: 12 additions & 12 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,22 +74,22 @@
"@babel/preset-env": "7.13.15",
"@babel/preset-react": "7.13.13",
"@fortawesome/fontawesome-free": "5.15.3",
"@storybook/addon-essentials": "6.2.9",
"@storybook/addon-essentials": "6.4.22",
"@storybook/addon-events": "6.2.9",
"@storybook/addon-knobs": "6.2.9",
"@storybook/addon-links": "6.2.9",
"@storybook/addon-links": "6.4.22",
"@storybook/addon-postcss": "2.0.0",
"@storybook/addon-storysource": "6.2.9",
"@storybook/addon-viewport": "6.2.9",
"@storybook/addons": "6.2.9",
"@storybook/cli": "6.2.9",
"@storybook/components": "6.2.9",
"@storybook/core-events": "6.2.9",
"@storybook/node-logger": "6.2.9",
"@storybook/addon-storysource": "6.4.22",
"@storybook/addon-viewport": "6.4.22",
"@storybook/addons": "6.4.22",
"@storybook/cli": "6.4.22",
"@storybook/components": "6.4.22",
"@storybook/core-events": "6.4.22",
"@storybook/node-logger": "6.4.22",
"@storybook/preset-scss": "1.0.3",
"@storybook/react": "6.2.9",
"@storybook/storybook-deployer": "2.8.7",
"@storybook/theming": "6.2.9",
"@storybook/react": "6.4.22",
"@storybook/storybook-deployer": "2.8.11",
"@storybook/theming": "6.4.22",
"@types/react": "17.0.3",
"@wojtekmaj/enzyme-adapter-react-17": "0.6.1",
"babel-core": "7.0.0-bridge.0",
Expand Down
20 changes: 20 additions & 0 deletions src/components/dropdown/__test__/__snapshots__/dropdown.test.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,8 @@ exports[`Dropdown component Should appear above the dropdown button 1`] = `
role="presentation"
>
<button
aria-controls="dropdown-menu"
aria-haspopup={true}
className="button"
tabIndex={0}
>
Expand Down Expand Up @@ -59,6 +61,8 @@ exports[`Dropdown component Should be right-aligned when using "right" prop 1`]
role="presentation"
>
<button
aria-controls="dropdown-menu"
aria-haspopup={true}
className="button"
tabIndex={0}
>
Expand Down Expand Up @@ -106,6 +110,8 @@ exports[`Dropdown component Should concat Bulma class with classes in props 1`]
role="presentation"
>
<button
aria-controls="dropdown-menu"
aria-haspopup={true}
className="button"
tabIndex={0}
>
Expand Down Expand Up @@ -150,6 +156,8 @@ exports[`Dropdown component Should have custom inline styles 1`] = `
role="presentation"
>
<button
aria-controls="dropdown-menu"
aria-haspopup={true}
className="button"
tabIndex={0}
>
Expand Down Expand Up @@ -194,6 +202,8 @@ exports[`Dropdown component Should have divider 1`] = `
role="presentation"
>
<button
aria-controls="dropdown-menu"
aria-haspopup={true}
className="button"
tabIndex={0}
>
Expand Down Expand Up @@ -244,6 +254,8 @@ exports[`Dropdown component Should have dropdown classname 1`] = `
role="presentation"
>
<button
aria-controls="dropdown-menu"
aria-haspopup={true}
className="button"
tabIndex={0}
>
Expand Down Expand Up @@ -273,6 +285,8 @@ exports[`Dropdown component Should have dropdown classname 1`] = `
</div>
`;

exports[`Dropdown component Should pass specified ID to the menu component 1`] = `ShallowWrapper {}`;

exports[`Dropdown component Should show custom label passed to the label prop 1`] = `
<div
className="dropdown"
Expand All @@ -283,6 +297,8 @@ exports[`Dropdown component Should show custom label passed to the label prop 1`
role="presentation"
>
<button
aria-controls="dropdown-menu"
aria-haspopup={true}
className="button"
tabIndex={0}
>
Expand Down Expand Up @@ -322,6 +338,8 @@ exports[`Dropdown component Should show the label of the first dropdown item whe
role="presentation"
>
<button
aria-controls="dropdown-menu"
aria-haspopup={true}
className="button"
tabIndex={0}
>
Expand Down Expand Up @@ -361,6 +379,8 @@ exports[`Dropdown component should also be right-aligned when using "align" prop
role="presentation"
>
<button
aria-controls="dropdown-menu"
aria-haspopup={true}
className="button"
tabIndex={0}
>
Expand Down
8 changes: 8 additions & 0 deletions src/components/dropdown/components/dropdown-context.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
import React from 'react';

const DropdownContext = React.createContext({
value: undefined,
onSelect: undefined,
});

export default DropdownContext;
9 changes: 7 additions & 2 deletions src/components/dropdown/components/item.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,17 +1,22 @@
import React from 'react';
import React, { useContext } from 'react';
import PropTypes from 'prop-types';
import classnames from 'classnames';

import Element from '../../element';
import DropdownContext from './dropdown-context';

const DropdownItem = ({ active, children, value, className, ...props }) => {
const context = useContext(DropdownContext);
return (
<Element
title={value}
onClick={() => {
context.onSelect(value);
}}
{...props}
role="presentation"
className={classnames(className, 'dropdown-item', {
'is-active': active,
'is-active': active || context.value === value,
})}
>
{children}
Expand Down
125 changes: 59 additions & 66 deletions src/components/dropdown/dropdown.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import DropdownDivider from './components/divider';
import Button from '../button';

import Element from '../element';
import DropdownContext from './components/dropdown-context';

const Dropdown = ({
className,
Expand All @@ -23,17 +24,15 @@ const Dropdown = ({
domRef,
disabled,
menuId,
fullwidth,
...props
}) => {
const style = fullwidth ? { width: '100%' } : undefined;
const ref = useRef(domRef);
const [isOpen, setIsOpen] = useState(false);
const close = (evt) => {
// IDK yet how to test using the ref in enzime
// istanbul ignore if
if (
hoverable ||
(evt && ref && ref.current && ref.current.contains(evt.target))
) {
if (hoverable || (evt && ref.current && ref.current.contains(evt.target))) {
return;
}
if (ref.current) {
Expand All @@ -42,14 +41,12 @@ const Dropdown = ({
};

const onSelect = (selectedValue) => {
return () => {
if (onChange) {
onChange(selectedValue);
}
if (closeOnSelect) {
close();
}
};
if (onChange) {
onChange(selectedValue);
}
if (closeOnSelect) {
close();
}
};

useEffect(() => {
Expand All @@ -59,63 +56,59 @@ const Dropdown = ({
};
}, []);

let current = label;

const childrenArray = React.Children.map(children, (child, i) => {
if (
child.type === DropdownItem &&
((i === 0 && !label) || child.props.value === value)
) {
current = child.props.children;
}
return React.cloneElement(
child,
child.type === DropdownItem
? {
active: child.props.value === value,
onClick: onSelect(child.props.value),
}
: {},
);
});
const currentLabel = React.Children.toArray(children)
.filter((child) => {
return child.type === DropdownItem;
})
.reduce((current, child, i) => {
if ((i === 0 && !current) || child.props.value === value) {
return child.props.children;
}
return current;
}, label);
Comment on lines +63 to +68
Copy link
Collaborator

Choose a reason for hiding this comment

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

What about using find instead? find stops iterating when it finds a match unlike reduce.

React.Children.toArray(children)
    .find((child, i) => child.type === DropdownItem && ((i === 0 && !label) || child.props.value === value)
    ?? label


return (
<Element
{...props}
domRef={ref}
className={classnames('dropdown', className, {
'is-active': isOpen,
'is-up': up,
'is-right': right || align === 'right',
'is-hoverable': hoverable,
})}
>
<div
className="dropdown-trigger"
role="presentation"
onClick={() => {
if (disabled) {
return;
}
setIsOpen((open) => {
return !open;
});
}}
<DropdownContext.Provider value={{ onSelect, value }}>
<Element
{...props}
domRef={ref}
className={classnames('dropdown', className, {
'is-active': isOpen,
'is-up': up,
'is-right': right || align === 'right',
'is-hoverable': hoverable,
})}
>
<Button
disabled={disabled}
color={color}
aria-haspopup
aria-controls={menuId}
<div
className="dropdown-trigger"
role="presentation"
style={style}
onClick={() => {
if (disabled) {
return;
}
setIsOpen((open) => {
return !open;
});
}}
>
<span>{current}</span>
{icon}
</Button>
</div>
<div className="dropdown-menu" id={menuId} role="menu">
<div className="dropdown-content">{childrenArray}</div>
</div>
</Element>
<Button
disabled={disabled}
color={color}
aria-haspopup
aria-controls={menuId}
fullwidth={fullwidth}
style={fullwidth ? { justifyContent: 'space-between' } : undefined}
>
<span>{currentLabel}</span>
{icon}
</Button>
</div>
<div className="dropdown-menu" id={menuId} role="menu" style={style}>
<div className="dropdown-content">{children}</div>
</div>
</Element>
</DropdownContext.Provider>
);
};

Expand Down
66 changes: 40 additions & 26 deletions src/components/dropdown/dropdown.story.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,37 +91,51 @@ Overview.argTypes = {
type: 'boolean',
},
},
fullwidth: {
defaultValue: false,
control: {
type: 'boolean',
},
},
};

export const Controlled = (args) => {
const [selected, setSelected] = useState('');
return (
<Dropdown {...args} value={selected} onChange={setSelected} color="info">
<Dropdown.Item renderAs="a" value="item">
Dropdown item
</Dropdown.Item>
<Dropdown.Item renderAs="a" value="other">
Other Dropdown item
</Dropdown.Item>
<Dropdown.Item renderAs="a" value="active">
Active Dropdown item
</Dropdown.Item>
<Dropdown.Item renderAs="a" value="other 2">
Other Dropdown item
</Dropdown.Item>
<Dropdown.Divider />
<Dropdown.Item renderAs="a" value="divider">
With divider
</Dropdown.Item>
</Dropdown>
<Box
style={{
display: 'flex',
justifyContent: 'center',
alignItems: 'center',
height: 600,
}}
>
<Dropdown
{...args}
value={selected}
style={{ width: 320 }}
onChange={setSelected}
icon={icon}
>
<Dropdown.Item renderAs="a" value="item">
Dropdown item
</Dropdown.Item>
<Dropdown.Item renderAs="a" value="other">
Other Dropdown item
</Dropdown.Item>
<Dropdown.Item renderAs="a" value="active">
Active Dropdown item
</Dropdown.Item>
<Dropdown.Item renderAs="a" value="other 2">
Other Dropdown item
</Dropdown.Item>
<Dropdown.Divider />
<Dropdown.Item renderAs="a" value="divider">
With divider
</Dropdown.Item>
</Dropdown>
</Box>
);
};

Controlled.argTypes = {
hoverable: {
defaultValue: false,
control: {
type: 'boolean',
},
},
};
Controlled.argTypes = Overview.argTypes;
Loading