Pd 12487 collapsible checklist #123

jamiek-acl · 2019-07-15T23:39:37Z

🛠 Purpose

Create 'CollapsibleChecklists' component.
Can have optional headings.
Can disable parent or child.
Clicking checkbox of parent selects all children.
Clicking a parent or child fires a callback, passing in the children to change.
Labels of parent and child can be any node (not just a string).

✏️ Notes to Reviewer

🖥 Screenshots

nahumzs · 2019-07-16T17:02:57Z

packages/Collapsible/package.json

@@ -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"


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

read more: #91

packages/CollapsibleChecklists/package.json

tristanjasper · 2019-07-16T22:49:56Z

packages/CollapsibleChecklists/src/CollapsibleChecklists.js

@@ -0,0 +1,41 @@
+import React from "react";
+import PropTypes from "prop-types";
+import Heading from "./components/Heading";


should import paprika components instead of relative paths

oh its not a paprika Heading, its my own

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

packages/CollapsibleChecklists/src/components/Heading/Heading.js

packages/CollapsibleChecklists/src/components/Item/Item.js

packages/Collapsible/src/Collapsible.js

packages/CollapsibleChecklists/README.md

tristanjasper · 2019-07-16T22:58:53Z

packages/CollapsibleChecklists/stories/examples/Basic.js

+        { isChecked: false, isDisabled: false, name: "Cleveland Browns" },
+      ];
+      setSportsData(newSportsData);
+    }, 2000);


create a static variable for this number eg const EXPAND_DURATION=2000 🤷‍♂

It's just a story; think that is necessary?

packages/CollapsibleChecklists/stories/examples/Basic.js

packages/CollapsibleChecklists/src/components/Heading/Heading.styles.js

packages/CollapsibleChecklists/stories/examples/Basic.js

tristanjasper · 2019-07-16T23:06:46Z

packages/CollapsibleChecklists/stories/examples/Basic.js

+  }
+
+  function renderOhioFootballTeams() {
+    if (sportsData[1].sports[0].teams.length === 0) {


perhaps create variables for the items with namings like const firstSport = sports[1].sports[0] so that can avoid setting the array index in multiple places and easy to follow

packages/CollapsibleChecklists/src/components/Heading/Heading.styles.js

alexzherdev · 2019-07-16T23:05:42Z

packages/CollapsibleChecklists/README.md

+```js
+import CollapsibleChecklists from "@paprika/collapsible-checklists";
+
+const yourComponent = () => {


Suggested change

const yourComponent = () => {

const YourComponent = () => {

alexzherdev · 2019-07-16T23:48:25Z

packages/CollapsibleChecklists/tests/CollapsibleChecklists.spec.js

+  it("cannot expand a disabled group", () => {
+    const { getByText, container } = renderComponent();
+    expect(getByText(/easter/i)).not.toBeVisible();
+    fireEvent.click(container.querySelectorAll(".collapsible__label")[2]); // Lilies


That class name shouldn't exist though?

Great catch

alexzherdev · 2019-07-17T00:00:18Z

packages/CollapsibleChecklists/src/CollapsibleChecklists.js

+      {children.map((child, index) => {
+        switch (child.type.displayName) {
+          case Heading.displayName:
+            return <Heading key={`heading${index}`} {...child.props} />; // eslint-disable-line react/no-array-index-key


Why is the key necessary? I think keys should be handled by the consumer. Keys represent the "identity" of elements, only consumer can know that. So this can just return child

The key is necessary for React console warning:

Warning: Each child in a list should have a unique "key" prop.

See if that's still there if you just return child itself?
It should also disappear for the Group case if you use React.cloneElement since that should preserve keys.

alexzherdev · 2019-07-17T00:14:09Z

packages/CollapsibleChecklists/src/CollapsibleChecklists.js

+  return (
+    <React.Fragment>
+      {children.map((child, index) => {
+        switch (child.type.displayName) {


Not sure what others think about using displayName here. Just checking child.type should work; it used to be broken by react-hot-loader but apparently that's not the case anymore? gaearon/react-hot-loader#304 (comment)

Can we count on the type not being changed during transpilation / minification?

Yes that identity should be preserved. Especially when there is no hot reloading involved, there should be nothing to worry about AFAIK.

I can remove the .displayName part

I'm kind of hesitant to recommend this though since that would require everyone to use the latest react-hot-loader... thoughts?

How many apps are using the latest react-hot-loader vs the broken one?

Since there are a few components now using this .displayName approach, I'm leaning towards keeping it for now, not creating issues in our apps, and capturing this possible refactor in an issue which we can revisit when at least most of our apps have updated their 🔥 loaders.

Some of them are not using react-hot-loader at all...

Sounds good. Looks like refactoring this will just be a semver-patch.

alexzherdev · 2019-07-17T00:14:49Z

packages/CollapsibleChecklists/src/CollapsibleChecklists.js

+          case Heading.displayName:
+            return <Heading key={`heading${index}`} {...child.props} />; // eslint-disable-line react/no-array-index-key
+          case Group.displayName:
+            return <Group key={`group${index}`} {...child.props} onChange={onChange} />; // eslint-disable-line react/no-array-index-key


This should use React.cloneElement

alexzherdev · 2019-07-17T00:48:06Z

packages/CollapsibleChecklists/src/components/Group/Group.js

+        onChange={toggleChildren}
+        aria-label={textContent(title)}
+      />
+      {title}


We should probably think about how to associate whatever the title is with the checkbox for accessibility @mikrotron

Well, that's what Jamie's trying to do with textContent(title), but as mentioned, that won't really work without rendering the node. Even the library Jamie is using will probably not work for us in all situations:

rwu823/react-addons-text-content#1 (comment)

So yeah, we should probably require a string version from the consumer as an a11yText prop.

... or ... maybe if there was a visuallyHidden <span id={someUID}>{title}</span>, and then we could have an aria-labelledby={someUID} on the input ... 🤔

But that wouldn't satisfy the requirement of checking the checkbox via clicking the label? Do we still want that?

Excellent point! In general clicking the label should activate the checkbox since that's such a small click target... it as a big impact on a11y! In this specific case though, I wonder... there's not an easy "undo" to the select all / deselect all operation of this checkbox, and maybe it's better to confine it to the actual checkbox. It could easily be assumed by the user that the title in this case would actually expand the Collapsible, and then they'd be frustrated.

I think we need a @krismckinnonacl for this one!

Agree with all the discussion here, but I think this can be addressed in another version, this is not the part of the API so we could refactor it easily in a bump it.

Can we create an issue with all the followed steps from this review @jamiek-acl and include this one?

I would make clicking the label activate that checkbox. For the following reasons:

Assuming that the child entries underneath work normally, if we did make that change, we'd be changing the behaviour of an element that looks visually the same, in close proximity to other controls that do the same thing, in a subtle way - I assume that clicking on the titles of the items underneath would check the associated checkbox, right?

If the user does in fact assume that clicking the title expands or collapses the section but instead selects all the elements, it's an easy mistake to correct, because they will encounter it BEFORE they select items underneath. It would be atypical for someone to

click the caret, then

select some things, then

click the caret to collapse again then

come back later and click the title instead of where they clicked before to open the elements.

Yes checking the titles of the children toggles them, too.

No it's not easily undone. If some children are selected and i click the parent, that would select all of its children. If you didn't intend that and wanted to undo, you wouldn't know which ones to de-select.

alexzherdev · 2019-07-17T00:49:19Z

packages/CollapsibleChecklists/src/components/Group/Group.js

+        if (isCollapsed && onExpand) {
+          onExpand();
+        }
+        setIsCollapsed(!isCollapsed);


Suggested change

setIsCollapsed(!isCollapsed);

setIsCollapsed(prevIsCollapsed => !prevIsCollapsed);

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

alexzherdev · 2019-07-17T00:52:28Z

packages/CollapsibleChecklists/stories/examples/Basic.js

+  return (
+    <CollapsibleChecklists onChange={handleOnChange}>
+      <CollapsibleChecklists.Heading>California Sports Teams</CollapsibleChecklists.Heading>
+      <CollapsibleChecklists.Group title={sportsData[0].sports[0].title}>


I think it'd be more representative of the real usage if this was created in a loop

I'm trying to keep it as simple as possible; I want to demonstrate to future onlookers that the API looks like this:

<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.Group title="Lilies" isDisabled> <CollapsibleChecklists.Item>Easter</CollapsibleChecklists.Item> <CollapsibleChecklists.Item>Madonna</CollapsibleChecklists.Item> <CollapsibleChecklists.Item>Stargazer</CollapsibleChecklists.Item> <CollapsibleChecklists.Item>Tiger</CollapsibleChecklists.Item> <CollapsibleChecklists.Item>Turks Cap</CollapsibleChecklists.Item> </CollapsibleChecklists.Group> </CollapsibleChecklists>

not have them spend time deciphering what all the loops are doing.

alexzherdev · 2019-07-17T01:09:46Z

packages/CollapsibleChecklists/src/components/Group/Group.js

+      childItemsToChange = childItems.filter(childItem => !childItem.props.isChecked && !childItem.props.isDisabled);
+    }
+
+    onChange(childItemsToChange);


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)

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

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.

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.

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

I see, I'm -1 on this.

alexzherdev · 2019-07-17T01:23:30Z

packages/CollapsibleChecklists/src/index.js

@@ -0,0 +1,3 @@
+import CollapsibleChecklists from "./CollapsibleChecklists";


Suggested change

import CollapsibleChecklists from "./CollapsibleChecklists";

export { default } from "./CollapsibleChecklists";

alexzherdev · 2019-07-18T17:47:26Z

@nahumzs this was merged too soon IMO. The reason I was asking for a real-world story is that I believe the current API will not work. Here's roughly how you'll have to use it in a loop:

<CollapsibleChecklists onChange={handleOnChange}>
  {sections.map(({ heading, items }) => (
    <React.Fragment key={heading}>
      <CollapsibleChecklists.Heading>{heading}</CollapsibleChecklists.Heading>
      <CollapsibleChecklists.Group>...</CollapsibleChecklists.Group>
    </React.Fragment>
  )}
</CollapsibleChecklists>

This doesn't work because we are forEaching through <CollapsibleChecklists>' children to find <Group>s to clone and we won't find any here.

Additionally, since we're fiddling with keys, we'd want a story where a consumer has their own keys on components to be sure we haven't broken anything for that case.

jamiek-acl · 2019-07-18T17:50:29Z

@alexzherdev why is the story that is included not a real-world story?

alexzherdev · 2019-07-18T17:54:33Z

Probably not worded well enough—I think subjectively components like this are more often data-driven than not? Meaning you'll have a collection of state to loop over (perhaps coming from an API), rather than hard code individual data pieces in. Even if we assume it's 50/50, still we want the component to work in those other 50% of cases.

jamiek-acl · 2019-07-18T18:13:01Z

The existing story simulates:

grabbing some of the data from an API (the Ohio Football group)
looping over that data and maintaining state of selected items/groups of items
disabled items
disabled groups
headings
items that are simple strings
items that are nodes

Check this video of the Storybook story that demonstrates those things, plus keyboard a11y:

alexzherdev · 2019-07-18T18:36:39Z

Ah, I didn't mean the actual end customer usage in the browser—that is perfectly covered in the story like that video demonstrates. I meant the usage of the component by developers consuming it in application code. The story code doesn't reflect the case when headings+groups are in an array (grabbed from an API) that has to be looped over, like in my snippet above. And if it did, you'd find that it doesn't work (unless I'm totally missing something).

alexzherdev · 2019-07-18T18:52:53Z

packages/CollapsibleChecklists/src/CollapsibleChecklists.js

+  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


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

jamiek-acl · 2019-07-19T16:53:06Z

@alexzherdev I've added another story to demonstrate grabbing headings and groups from an API, that Nahum created the other day: #134

PD-12487 Make CollapsibleChecklists component

52f57fb

jamiek-acl force-pushed the pd-12487-collapsible-checklist branch from fa0fac7 to 52f57fb Compare July 15, 2019 23:43

nahumzs reviewed Jul 16, 2019

View reviewed changes

packages/CollapsibleChecklists/package.json Outdated Show resolved Hide resolved

jamiek-acl added 2 commits July 16, 2019 10:44

PD-12487 Add styles

61fd8ea

PD-12487 Add tests, Readme, clean-up

2192f4d

jamiek-acl marked this pull request as ready for review July 16, 2019 21:33

Merge branch 'master' into pd-12487-collapsible-checklist

df065fe