Add Pill Container: Listbox of Pill Options component #1645

kevinparkerson · 2018-11-12T05:49:28Z

Fixes #1362

Following PR review, I've created a "Pill Container" component, addressing the following:

Move listbox-of-pill-options.jsx to it's own "PillContainer" folder
Change name of "onRemovePill" to "onRequestRemovePill" and preserve focus after an item is removed
Add variant:base, bare that enables using the bare modifier for the whole pill container.
Add style and className props
Options should be an array with objects a defined shape
Switch data examples to react components (icon, avatar)
Allow icon and avatar to be regular nodes as well

Additionally, I found a bug in the combobox when attempting to tab to the selected items and fixed it :)

CONTRIBUTOR checklist (do not remove)

Please complete for every pull request

First-time contributors should sign the Contributor License Agreement. It's a fancy way of saying that you are giving away your contribution to this project. If you haven't before, wait a few minutes and a bot will comment on this pull request with instructions.
npm run lint:fix has been run and linting passes.
Mocha, Jest (Storyshots), and components/component-docs.json CI tests pass (npm test).
Tests have been added for new props to prevent regressions in the future. See readme.
Review the appropriate Storybook stories. Open http://localhost:9001/.
The Accessibility panel of each Storybook story has 0 violations (aXe). Open http://localhost:9001/.
Review tests are passing in the browser. Open http://localhost:8001/.
Review markup conforms to SLDS by looking at DOM snapshot strings.

REVIEWER checklist (do not remove)

TravisCI tests pass. This includes linting, Mocha, Jest, Storyshots, and components/component-docs.json tests.
Tests have been added for new props to prevent regressions in the future. See readme.
Review the appropriate Storybook stories. Open http://localhost:9001/.
The Accessibility panel of each Storybook story has 0 violations (aXe). Open http://localhost:9001/.
Review tests are passing in the browser. Open http://localhost:8001/.
Review markup conforms to SLDS by looking at DOM snapshot strings.

Required only if there are markup / UX changes

Add year-first date and commit SHA to last-slds-markup-review in package.json and push.
Request a review of the deployed Heroku app by the Salesforce UX Accessibility Team.
Add year-first review date, and commit SHA, last-accessibility-review, to package.json and push.
While the contributor's branch is checked out, run npm run local-update within locally cloned site repo to confirm the site will function correctly at the next release.

… and made it preserve focus following removal

…tions array objects, added support for error pills, added className and style props

interactivellama · 2018-11-27T23:10:02Z

@vargasd We should be able to get Listbox of Pill Options component to you by the end of the week.

interactivellama · 2018-11-28T05:39:46Z

components/combobox/combobox.jsx

+	handleKeyDownTab = () => {
+		if (this.selectedListboxRef) {
+			this.setState({
+				activeSelectedOption: this.props.selection[0],


What's the reason for the tab reset here? We are not doing it in PillContainer right now either http://localhost:9001/?selectedKind=SLDSPillContainer&selectedStory=Base%20Pill%20Container&full=0&addons=1&stories=1&panelRight=0&addonPanel=storybook%2Factions%2Factions-panel

@interactivellama if you are focused on the combobox input, pressing tab should move focus to the listbox of pills. Before this code was added it would switch away from the entire combobox (and listbox) altogether. Pressing tab while in the listbox moves focus away to the next item in the page, which I believe is correct behavior. This code isn't in PillContainer because it is not attached to a combobox input like in this case.

interactivellama

A few question here and there, but component is manually testing well.

I'm unsure about the reason to return focus to the first pill after tabbing forward.

interactivellama · 2018-11-28T06:01:35Z

components/pill-container/__docs__/site-stories.js

+/* eslint-disable global-require */
+
+const siteStories = [
+	require('raw-loader!@salesforce/design-system-react/components/pill/__examples__/base.jsx'),


These should be importing from pill-container

interactivellama · 2018-11-28T06:24:07Z

components/pill-container/__examples__/bare.jsx

+		this.state = {
+			options: [
+				{
+					bare: true,


I think bare is set by the container and not on the pill.

@interactivellama it can be both. The SLDS Listbox of Pills .slds-pill_bare modifier shows a class on the pill itself, but there's also a .slds-pill_container_bare class for the container. See below:

interactivellama · 2018-11-28T06:37:37Z

components/pill-container/__tests__/pill-container.browser-test.jsx

+		afterEach(unmountComponent);
+
+		it('Renders the base Pill Container correctly', function() {
+			let currentPill = 1;


Could we update this to currentPillId?

I thought that said "current pilld" 😆I will probably call it idOfCurrentPill to make it easier to read

interactivellama · 2018-11-28T06:41:51Z

components/pill-container/__tests__/pill-container.browser-test.jsx

+			});
+		});
+	});
+});


We'll need some focus detection to ensure the keyboard arrow keys and del/backspace work. These should be able to be directly copied from https://github.com/salesforce/design-system-react/blob/master/components/combobox/__tests__/combobox.browser-test.jsx#L261

@interactivellama had to do them a bit differently here, but I added them!

interactivellama · 2018-11-28T06:43:17Z

components/pill-container/__tests__/pill-container.snapshot-test.jsx

@@ -0,0 +1,32 @@
+/* eslint-env jest */


This file shouldn't be needed. Storyshots should take care of this. https://github.com/salesforce/design-system-react/blob/master/components/story-based-tests.js#L17

interactivellama · 2018-11-28T06:43:38Z

components/pill-container/docs.json

@@ -0,0 +1,7 @@
+{
+    "component": "pill-listbox",


Please update the name.

interactivellama · 2018-11-28T06:46:29Z

components/pill-container/index.jsx

+					variant: PropTypes.string,
+				}),
+			]),
+			bare: PropTypes.bool,


This should be set by the parent/PillContainer

@interactivellama we need to discuss this as there is a discrepancy on SLDS in regards to the bare pill class and bare container class

interactivellama · 2018-11-28T06:49:57Z

components/pill-container/index.jsx

+		const skipIndex =
+			options.length > nextIndex &&
+			nextIndex >= 0 &&
+			options[nextIndex].type === 'separator';


There shouldn't be any separator.

interactivellama · 2018-11-28T06:59:59Z

components/pill-container/index.jsx

+		const options = this.props.options;
+		let activeSelectedOptionIndex = this.state.activeSelectedOptionIndex;
+
+		while (


I would prefer a foreach or map here for the loop.

interactivellama · 2018-11-28T07:04:34Z

components/pill-container/private/selected-listbox.jsx

@@ -94,20 +179,16 @@ const SelectedListBox = (props) =>
 			>
 				{props.selection.map((option, renderIndex) => {
 					const setActiveBasedOnstateFromParent =
-						renderIndex === props.activeOptionIndex &&
-						isEqual(option, props.activeOption);


What's the reason for removal of this line?

@interactivellama it's actually a modification to:
const setActiveBasedOnStateFromParent = renderIndex === props.activeOptionIndex;

It's unnecessary to check whether an individual option in a particular iteration deeply equals the activeOption as props.selection is an array and therefore consistently ordered, so renderIndex is always reliable when compared to props.activeOptionIndex. Since activeOption and activeOptionIndex are always set at the same time by the parent combobox and pill-container components and selected-listbox is a private component, it's impossible for those two props to ever get out of sync. It also didn't seem appropriate for the selected-listbox to care about the contents of a particular option while managing focus, which only requires position. Ultimately simplifying that line makes it easier to read and marginally improves performance.

…avior correct for combobox, fix a bug with aria-selected, and more

interactivellama · 2018-11-29T04:51:14Z

components/combobox/__tests__/__snapshots__/combobox.snapshot-test.jsx.snap

        >
          <span
-            aria-selected={true}
+            aria-selected={false}


This should be true, since tabIndex="0"

The pill that can be tabbed to from outside the component has tabIndex="0" and it can have that value before focus is actually on the pill. Hopefully that's correct behavior? I added the onPillFocus stuff to make sure that when the pill was tabbed to it properly reflected aria-selected={true} at that point.

“~” which is used for string replacement, should only be used in examples for the doc site

Update package.json, Add clarity to component descriptions Build docs

salesforce#1362 Adds Listbox of Pill Options component

195a4b8

kevinparkerson requested a review from interactivellama November 12, 2018 05:49

PR updates for listbox-of-pill-options

9f07fd9

interactivellama added the pills label Nov 19, 2018

kparkerson added 4 commits November 21, 2018 17:36

created pill-container component from listbox-of-pill-options

c85591c

changed pill-container onRemovePill method to onRequestRemovePill…

db96742

… and made it preserve focus following removal

pill-container: added 'base' and 'bare' variants, defined shape of op…

ca4b1bb

…tions array objects, added support for error pills, added className and style props

pill-container: examples and tests updated

9cec2ca

kevinparkerson changed the title ~~Adds Listbox of Pill Options component~~ Adds Listbox of Pill Options component (called "Pill Container") Nov 22, 2018

interactivellama reviewed Nov 28, 2018

View reviewed changes

interactivellama requested changes Nov 28, 2018

View reviewed changes

kparkerson added 2 commits November 28, 2018 12:39

more PR updates to pill-container

26139c6

even more PR updates to pill-container to improve tests, make tab beh…

a73568a

…avior correct for combobox, fix a bug with aria-selected, and more

interactivellama reviewed Nov 29, 2018

View reviewed changes

interactivellama temporarily deployed to design-system-react-co-pr-1645 November 29, 2018 19:56 Inactive

interactivellama added 4 commits November 29, 2018 23:43

Combobox/Pill Container: Revert inital active state

8212cb4

Pill Container: Add descriptions to props

6b51e40

Pill: Update import paths

8ab3f87

“~” which is used for string replacement, should only be used in examples for the doc site

Pill/Pill Container: Add doc site data, add prop descriptions

3d250f9

Update package.json, Add clarity to component descriptions Build docs

interactivellama temporarily deployed to design-system-react-co-pr-1645 November 30, 2018 07:05 Inactive

Pill Container: Remove single component DOM snapshots

180065d

interactivellama temporarily deployed to design-system-react-co-pr-1645 November 30, 2018 07:10 Inactive

PillContainer: Update grouped DOM snapshots

2913ee0

interactivellama deployed to design-system-react-co-pr-1645 November 30, 2018 07:44 View deployment

interactivellama approved these changes Nov 30, 2018

View reviewed changes

interactivellama merged commit d209d2e into salesforce:master Nov 30, 2018

interactivellama changed the title ~~Adds Listbox of Pill Options component (called "Pill Container")~~ Add Pill Container: Listbox of Pill Options component Nov 30, 2018

interactivellama mentioned this pull request Dec 1, 2018

Pill: Remove checkprops since it is production now #1663

Merged

Add Pill Container: Listbox of Pill Options component #1645

Add Pill Container: Listbox of Pill Options component #1645

Uh oh!

Conversation

kevinparkerson commented Nov 12, 2018 • edited by interactivellama Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

CONTRIBUTOR checklist (do not remove)

REVIEWER checklist (do not remove)

Required only if there are markup / UX changes

Uh oh!

interactivellama commented Nov 27, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kevinparkerson Nov 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

interactivellama left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kevinparkerson Nov 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

kevinparkerson commented Nov 12, 2018 •

edited by interactivellama

Loading

kevinparkerson Nov 28, 2018 •

edited

Loading

kevinparkerson Nov 28, 2018 •

edited

Loading