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.

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

Pd 12487 collapsible checklist #123

Merged
merged 20 commits into from
Jul 18, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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/Collapsible/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
"main": "lib/index.js",
"repository": {
"type": "git",
"url": "https://github.com/acl-services/paprika/tree/master/packages/collapsible"
"url": "https://github.com/acl-services/paprika/tree/master/packages/Collapsible"
Copy link
Contributor

@nahumzs nahumzs Jul 16, 2019

Choose a reason for hiding this comment

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

I think we should use homepage for this value and url property should point to the repo itself.

read more: #91

},
"publishConfig": {
"access": "public"
Expand Down
4 changes: 2 additions & 2 deletions packages/Collapsible/src/Collapsible.js
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ const Collapsible = props => {
const { a11yText, label, isCollapsed, isDisabled, iconAlign } = props;

return (
<React.Fragment>
<div data-pka-anchor="collapsible.heading">
<RawButton
ariaLabel={a11yText || I18n.t("collapsible.a11yText")}
aria-expanded={!isCollapsed}
Expand All @@ -85,7 +85,7 @@ const Collapsible = props => {
</span>
</RawButton>
{label}
</React.Fragment>
</div>
);
};

Expand Down
47 changes: 47 additions & 0 deletions packages/CollapsibleChecklists/README.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
## CollapsibleChecklists

A controlled component that has checkable groups of checkable items.
Checking the parent checks all of the children.

### Installation

`> npm install --save @paprika/collapsible-checklists`
or
`> yarn add @paprika/collapsible-checklists`

### Usage

```js
import CollapsibleChecklists from "@paprika/collapsible-checklists";

const YourComponent = () => {
return (
<CollapsibleChecklists
onChange={() => {
/*...*/
}}
>
<CollapsibleChecklists.Heading>Flowers</CollapsibleChecklists.Heading>

<CollapsibleChecklists.Group title="Roses">
<CollapsibleChecklists.Item isChecked>Damask</CollapsibleChecklists.Item>
<CollapsibleChecklists.Item isChecked>Eden</CollapsibleChecklists.Item>
<CollapsibleChecklists.Item isChecked>Lady Banks</CollapsibleChecklists.Item>
</CollapsibleChecklists.Group>

<CollapsibleChecklists.Group title="Irises">
<CollapsibleChecklists.Item isChecked>Siberian</CollapsibleChecklists.Item>
<CollapsibleChecklists.Item>Yellow</CollapsibleChecklists.Item>
<CollapsibleChecklists.Item isDisabled>Bearded</CollapsibleChecklists.Item>
<CollapsibleChecklists.Item>Netted</CollapsibleChecklists.Item>
</CollapsibleChecklists.Group>
</CollapsibleChecklists>
);
};

export default YourComponent;
```

### Props

- `onChange`
27 changes: 27 additions & 0 deletions packages/CollapsibleChecklists/package.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
{
"name": "@paprika/collapsible-checklists",
"version": "0.1.0",
"description": "CollapsibleChecklists component",
"author": "@paprika",
"license": "MIT",
"main": "lib/index.js",
"repository": {
"type": "git",
"url": "https://github.com/acl-services/paprika/tree/master/packages/CollapsibleChecklists"
},
"publishConfig": {
"access": "public"
},
"dependencies": {
"@babel/runtime-corejs2": "^7.3.1",
"@paprika/collapsible": "^0.1.0",
"@paprika/stylers": "^0.1.1",
"@paprika/tokens": "^0.1.1",
"prop-types": "^15.7.2"
},
"peerDependencies": {
"react": "^16.8.0",
"react-dom": "^16.8.0",
"styled-components": "^4.2.0"
}
}
39 changes: 39 additions & 0 deletions packages/CollapsibleChecklists/src/CollapsibleChecklists.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
import React from "react";
import PropTypes from "prop-types";
import Heading from "./components/Heading";
Copy link
Contributor

Choose a reason for hiding this comment

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

should import paprika components instead of relative paths

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh its not a paprika Heading, its my own

Copy link
Contributor

Choose a reason for hiding this comment

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

Wondering if perhaps we should try to not call our own components the same as paprika component names in general to avoid that confusion

import Group from "./components/Group";
import Item from "./components/Item";

const propTypes = {
children: PropTypes.node.isRequired,
onChange: PropTypes.func.isRequired,
};

const defaultProps = {};

const CollapsibleChecklists = props => {
const { children, onChange } = props;
const modifiedChildren = [];

React.Children.forEach(children, (child, index) => {
switch (child.type.displayName) {
case Group.displayName:
modifiedChildren.push(React.cloneElement(child, { key: `group${index}`, onChange })); // eslint-disable-line react/no-array-index-key
Copy link
Contributor

Choose a reason for hiding this comment

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

That ESLint rule speaks the truth. Even if you're not explicitly reordering items a la <Sortable>, when an item gets removed from the middle of the list things will break. https://reactjs.org/docs/lists-and-keys.html#keys

break;
default:
modifiedChildren.push(child);
}
});

return <React.Fragment>{modifiedChildren}</React.Fragment>;
};

CollapsibleChecklists.displayName = "CollapsibleChecklists";
CollapsibleChecklists.propTypes = propTypes;
CollapsibleChecklists.defaultProps = defaultProps;

CollapsibleChecklists.Heading = Heading;
CollapsibleChecklists.Group = Group;
CollapsibleChecklists.Item = Item;

export default CollapsibleChecklists;
129 changes: 129 additions & 0 deletions packages/CollapsibleChecklists/src/components/Group/Group.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,129 @@
import React from "react";
import PropTypes from "prop-types";
import Collapsible from "@paprika/collapsible";
import Item from "../Item";
import groupStyles from "./Group.styles";

const propTypes = {
children: PropTypes.node, // probably an array of "Items", but could be a Spinner or anything else
isDisabled: PropTypes.bool,
onChange: PropTypes.func,
onExpand: PropTypes.func,
title: PropTypes.node.isRequired,
};

const defaultProps = {
children: [],
isDisabled: false,
onChange: () => {},
onExpand: () => {},
};

function useIsIndeterminate(checkboxRef) {
const [, setIsIndeterminate] = React.useState(null);
React.useEffect(() => {
setIsIndeterminate(checkboxRef.current.indeterminate);
}, [checkboxRef.current.indeterminate]);
}

function Group(props) {
const { children, isDisabled, onChange, onExpand, title } = props;
const [isCollapsed, setIsCollapsed] = React.useState(true);
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps in the future we might want to have an initial value for this

const checkboxRef = React.useRef({});
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
const checkboxRef = React.useRef({});
const checkboxRef = React.useRef(null);

is the recommended way to initialize refs for DOM components

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@alexzherdev this does not work as I am calling checkboxRef.current.indeterminate

useIsIndeterminate(checkboxRef);
let allAreChecked = React.Children.count(props.children) > 0;
let noneAreChecked = true;

React.Children.forEach(children, child => {
if (child.props.isChecked) {
noneAreChecked = false;
} else {
allAreChecked = false;
}
});

if (allAreChecked || noneAreChecked) {
checkboxRef.current.indeterminate = false;
} else if (!allAreChecked && !noneAreChecked) {
checkboxRef.current.indeterminate = true;
}

function toggleChildren() {
const childItems = [];
React.Children.forEach(children, child => {
if (child.type === Item) {
childItems.push(child);
}
});

if (childItems.length === 0) {
return;
}

const allChildItemsAreChecked =
childItems.filter(childItem => childItem.props.isChecked || childItem.props.isDisabled).length ===
childItems.length;

let childItemsToChange = [];
if (allChildItemsAreChecked) {
childItemsToChange = childItems.filter(childItem => !childItem.props.isDisabled);
} else {
childItemsToChange = childItems.filter(childItem => !childItem.props.isChecked && !childItem.props.isDisabled);
}

onChange(childItemsToChange);
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm interested in hearing others' feedback about this. While discussing the API for <Sortable> with @mikrotron I said that an onChange API based on React elements doesn't make a lot of sense to me. In this case, given a set of React elements, how would the consumer know what to do? They would need to somehow map those elements to their actual data to determine what is being checked/unchecked. The storybook story doesn't really handle this, so it's not obvious that there's an issue.

We might want to require some kind of id prop on the items to handle this. (can't use key because that wouldn't be visible to us)

Copy link
Contributor

Choose a reason for hiding this comment

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

Since we expect an array, maybe just the indexes would be sufficient?

Copy link
Contributor

@nahumzs nahumzs Jul 17, 2019

Choose a reason for hiding this comment

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

The idea is that you always received the selectedIndexes as the first argument, and we could pass the full list of elements as second argument so you could do:

function handleChange(indexes, list) {
  indexes.forEach(index => console.log(list[index].props.value))
}

where value can be any property they want to access, can be id, data, etc...

of course, if we want to get the ones that haven't been selected we would need to use filter instead of map or forEach.

Copy link
Contributor

Choose a reason for hiding this comment

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

We only have a single onChange prop for the entire component now, so a single index would not be enough to describe a child checkbox, you'd also need an index of the group. (Alternatively, put a separate onChange prop on each group—which might be too repetitive though? Realistically a consumer is more likely to define a single callback handler for this component anyway I think?)

For the snippet above, seems like the list is redundant? The consumer is the one passing in those "values" in the first place, so they don't really need to be given them back. They already have some kind of state that they can iterate over.

I suggest creating a story that would better reflect the real world usage of this component (i.e. resembling the feature that will need it). That should make it easier to design.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The consumer can compare on anything... I chose to compare by key (as I set key={team.name} but could have just as easily called it foo={team.name}. In fact i'll update the example to use foobar, and the comparison checks props.foobar

Copy link
Contributor

Choose a reason for hiding this comment

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

I see, I'm -1 on this.

}

/* eslint-disable jsx-a11y/label-has-associated-control */
const label = (
<React.Fragment>
<label>
<input
ref={checkboxRef}
checked={allAreChecked}
type="checkbox"
disabled={isDisabled}
onChange={toggleChildren}
/>
{title}
</label>
</React.Fragment>
);
/* eslint-enable jsx-a11y/label-has-associated-control */

const modifiedChildren = [];
React.Children.forEach(children, (child, index) => {
const newProps = {
key: `${title}-child-${index}`,
};

if (child.type.displayName === Item.displayName) {
newProps.onChange = () => onChange([child]);
}

modifiedChildren.push(React.cloneElement(child, newProps));
});

return (
<Collapsible
css={groupStyles}
hasOnlyIconToggle
isCollapsed={isCollapsed}
isDisabled={isDisabled}
label={label}
onClick={() => {
if (isCollapsed && onExpand) {
onExpand();
}
setIsCollapsed(!isCollapsed);
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
setIsCollapsed(!isCollapsed);
setIsCollapsed(prevIsCollapsed => !prevIsCollapsed);

https://reactjs.org/docs/hooks-reference.html#functional-updates

}}
>
{modifiedChildren}
</Collapsible>
);
}

Group.displayName = "Group";
Group.propTypes = propTypes;
Group.defaultProps = defaultProps;
export default Group;
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
import tokens from "@paprika/tokens";
import { css } from "styled-components";
import { disabledStyle } from "../helpers.styles";

const groupStyles = css`
${({ isDisabled }) => {
return `
[data-pka-anchor="collapsible.heading"] {
align-items: center;
background-color: ${tokens.color.cremeLighten5};
border-bottom: 1px solid ${tokens.color.blackLighten60};
display: flex;
padding: ${tokens.spaceSm};

input[type="checkbox"] {
margin-right: ${tokens.space};
}

${isDisabled ? disabledStyle : ""};
}
`;
}}
`;

export default groupStyles;
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
export { default } from "./Group";
17 changes: 17 additions & 0 deletions packages/CollapsibleChecklists/src/components/Heading/Heading.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
import React from "react";
import PropTypes from "prop-types";
import headingStyles from "./Heading.styles";

const propTypes = {
children: PropTypes.node.isRequired,
};

const defaultProps = {};

const Heading = ({ children }) => <div css={headingStyles}>{children}</div>;

Heading.displayName = "Heading";
Heading.propTypes = propTypes;
Heading.defaultProps = defaultProps;

export default Heading;
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
import tokens from "@paprika/tokens";
import { css } from "styled-components";
import stylers from "@paprika/stylers";

const headingStyles = css`
background-color: ${tokens.table.header.backgroundColor};
border-bottom: 2px solid ${tokens.border.color};
font-weight: bold;
padding: ${tokens.space} ${tokens.space} ${tokens.space} ${stylers.spacer(2)};
`;

export default headingStyles;
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
export { default } from "./Heading";
37 changes: 37 additions & 0 deletions packages/CollapsibleChecklists/src/components/Item/Item.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
import React from "react";
import PropTypes from "prop-types";
import itemStyles from "./Item.styles";

const propTypes = {
children: PropTypes.node.isRequired,
isChecked: PropTypes.bool,
isDisabled: PropTypes.bool,
onChange: PropTypes.func,
};

const defaultProps = {
isChecked: false,
isDisabled: false,
onChange: () => {},
};

function Item(props) {
const { children, isChecked, isDisabled, onChange } = props;

/* eslint-disable jsx-a11y/label-has-associated-control */
return (
<div css={itemStyles} isDisabled={isDisabled}>
<label>
<input type="checkbox" checked={isChecked} disabled={isDisabled} onChange={onChange} />
{children}
</label>
</div>
);
/* eslint-enable jsx-a11y/label-has-associated-control */
}

Item.displayName = "Item";
Item.propTypes = propTypes;
Item.defaultProps = defaultProps;

export default Item;
19 changes: 19 additions & 0 deletions packages/CollapsibleChecklists/src/components/Item/Item.styles.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
import stylers from "@paprika/stylers";
import tokens from "@paprika/tokens";
import { css } from "styled-components";
import { disabledStyle } from "../helpers.styles";

const itemStyles = css`
${({ isDisabled }) => {
return `
padding: 2px 0 2px ${stylers.spacer(5)};
${isDisabled ? disabledStyle : ""};

input {
margin-right: ${tokens.space}
}
`;
}}
`;

export default itemStyles;
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
export { default } from "./Item";
Loading