feat(Select): add checkbox select variant #1487

kmcfaul · 2019-03-04T18:02:33Z

What: Adds checkbox select variant component.

Refer to issue: #1217

patternfly-build · 2019-03-04T18:12:23Z

PatternFly-React preview: https://1487-pr-patternfly-react-patternfly.surge.sh

codecov-io · 2019-03-04T18:18:56Z

Codecov Report

Merging #1487 into master will decrease coverage by 0.08%.
The diff coverage is 64.58%.

@@            Coverage Diff             @@
##           master    #1487      +/-   ##
==========================================
- Coverage    82.8%   82.72%   -0.09%     
==========================================
  Files         591      594       +3     
  Lines        6550     6594      +44     
  Branches       75       75              
==========================================
+ Hits         5424     5455      +31     
- Misses       1096     1109      +13     
  Partials       30       30

Flag	Coverage Δ
#patternfly3	`84.78% <ø> (ø)`	⬆️
#patternfly4	`79.46% <64.58%> (-0.15%)`	⬇️
#patternflymisc	`95.68% <ø> (ø)`	⬆️

Impacted Files	Coverage Δ
...rnfly-4/react-core/src/components/Select/Select.js	`93.33% <ø> (ø)`	⬆️
...eact-core/src/components/Select/selectConstants.js	`100% <ø> (ø)`	⬆️
...4/react-core/src/components/Select/SelectToggle.js	`54.05% <0%> (ø)`	⬆️
...-core/src/components/Select/CheckboxSelectGroup.js	`100% <100%> (ø)`
...core/src/components/Select/CheckboxSelectOption.js	`40% <40%> (ø)`
...react-core/src/components/Select/CheckboxSelect.js	`94.44% <94.44%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b3c406f...b24fa18. Read the comment docs.

mcoker · 2019-03-04T22:30:37Z

Hey @kmcfaul looks good! Compared to the core example...

.pf-c-select__menu-group should wrap the entire group of elements in the group.
In .pf-c-select__toggle, we have .pf-c-select__toggle-wrapper + .pf-c-select__toggle-badge + .pf-c-select__toggle-arrow

Is there any reason you wouldn't match this structure in the react component?

@jgiardino looking at this, do you agree that we should create an issue in core to update .pf-c-select__menu-group to be a section, and .pf-c-select__menu-group-title to be an h1 by default, like the dropdown component?

jgiardino · 2019-03-05T15:29:25Z

Great work on this, @kmcfaul!

There is currently a PR open in core to change how <fieldset> is used: patternfly/patternfly#1483

Would it be possible to follow what's done in that PR even though it isn't merged yet? This PR doesn't change the css, just the html structure to use separate fieldsets for each section of checkboxes.

When there are a group of checkboxes, the best way to group them is with a <fieldset>, and the label used for the fieldset can provide additional context when the checkbox label is announced (depending on screen reader and nav method). In this case, instead of using <legend>, we're using aria-labelledby to reference the visible label, and also are making that node hidden to screen readers to avoid duplication.

@jgiardino looking at this, do you agree that we should create an issue in core to update .pf-c-select__menu-group to be a section, and .pf-c-select__menu-group-title to be an h1 by default, like the dropdown component?

I would stick to the updates in that core PR, rather than incorporate section and h1 elements, since they would provide no added benefit. The h1 would be ignored due to aria-hidden, and the fieldset provides sufficient sectioning, and it could be annoying in some screen readers to wrap it with another section element.

jgiardino · 2019-03-05T15:38:08Z

Is this component also expected to work when there is just one group of checkboxes? I removed one of the groups, and removed the group title, and it rendered like this. This is actually very similar to the example in core, except that there's this empty div that adds extra space:

<div class="pf-c-select__menu-group"><div class="pf-c-select__menu-group-title"></div></div>

In this case, if the consumer has a single group with no label, then can we pull the <fieldset> label from the toggle?

jgiardino · 2019-03-05T15:42:48Z

packages/patternfly-4/react-core/src/components/Select/CheckboxSelectOption.js

+              className={css(checkStyles.checkInput)}
+              type="checkbox"
+              onChange={event => {
+                if (!isDisabled) {


There should also be a disabled attribute on the <input>.

jgiardino · 2019-03-05T15:51:30Z

Looking at the aria-labels, there are a couple of things I notice:

The example includes aria-label="Select Input", but I don't see this string included in the component
The Single Select example includes ariaLabelledBy={titleId} which includes an additional id in the aria-labelledby attribute for the toggle. We should do the same with this component.

kmcfaul · 2019-03-05T17:02:27Z

@mcoker I will address these. I was following the integration document for structure at the outset and may have misread it. For the toggle, I had read it as using the Badge component and didn't want to couple the components, but now I realize I can create a span and reuse the classes instead.

@jgiardino
Looks like I just need to create a fieldset under each pf-c-select__menu-group, if the select is grouped. I should be able to adjust the child mapping to include that.

For only one group of checkboxes without a label, the implementation would use an array of CheckboxSelectOption and leave the CheckboxSelectGroup out altogether. It is an optional wrapper and is flagged by the Select isGrouped prop. I will add another example to match core, showing a non-grouped checkbox select.

The aria-label="Select Input is getting passed down to the fieldset at the moment. With the fieldset changes though, I will move the prop to the outer wrapping div pf-c-select__menu. Let me know if the label should be applied to something different instead.

I will add the hidden title as in the SingleSelect example.

kmcfaul · 2019-03-05T17:31:09Z

@mcoker It looks like adjusting the badge html has some side effects, seemingly due to the width being set for Select. The toggle contents are justified so the badge floats more or less in the middle opposed to a specific side.

If I put the badge div under the toggle wrapper it floats closer to the text, but I'm not sure if it should be close to the text or to the carat.

Thoughts?

jgiardino · 2019-03-05T17:35:56Z

The aria-label="Select Input is getting passed down to the fieldset at the moment. With the fieldset changes though, I will move the prop to the outer wrapping div pf-c-select__menu. Let me know if the label should be applied to something different instead.

Oh, I see. For this component, I would suggest leaving the prop out, and only providing aria-label/labelledby attributes on the <fieldset> elements. For the case where there's only one <fieldset>, I would have the label come from the toggle.

mcoker · 2019-03-06T14:51:26Z

@kmcfaul Ah, unfortunately that's a bug in core, too.

My inclination is to match what's in core, even if it's broken, then we can fix it in core and hopefully it's just CSS that will fix it here. I mentioned this to @christiemolloy in an open PR about select - patternfly/patternfly#1527 (comment)

Either we'll fix it there, or once that's merged, we'll follow up and fix it then.

kmcfaul · 2019-03-08T18:34:58Z

@mcoker For the fieldset html structure change, it looks like there's now a grid gap between groups from the wrapping form. Is there something I can do to suppress it or is an expected adjustment?

@jgiardino For the aria changes in the fieldsets, I'm using the group label as the aria-labelledby property of each fieldset (and when there are no groups, the previously passed in aria-label is used).

mcoker · 2019-03-08T19:36:42Z

@mcoker For the fieldset html structure change, it looks like there's now a grid gap between groups from the wrapping form. Is there something I can do to suppress it or is an expected adjustment?

Looks like you're missing a .pf-c-form__group as a direct child of .pf-c-form

jgiardino · 2019-03-11T20:56:47Z

For the aria changes in the fieldsets, I'm using the group label as the aria-labelledby property of each fieldset (and when there are no groups, the previously passed in aria-label is used).

This approach sounds good. I just have a couple of comments/questions:

In the example below, where id="Status" is included, if the consumer defines something like "Lorem ipsum" as the group label, this gets generated as id="Lorem ipsum" with a space, which isn't a valid id value. Can we convert the string to a valid value? Are there any standards we follow regarding syntax of id values that we should follow?
The id should be on the .pf-c-select__menu-group-title element, not the .pf-c-select__menu-group. The .pf-c-select__menu-group-title has only the text that we want to use, but .pf-c-select__menu-group has the text plus all the checkbox labels.

<div id="Status" class="pf-c-select__menu-group">
  <div class="pf-c-select__menu-group-title">Status</div>
  <fieldset aria-labelledby="Status" class="pf-c-form__fieldset">

tlabaj · 2019-03-11T16:25:01Z

packages/patternfly-4/react-core/src/components/Select/Select.d.ts

  isExpanded?: boolean;
+  isGrouped?: boolean;
  onToggle(value: boolean): void;
-  placeholderText?: string;


We need to maintain the API and not cause breaking changes because we are not ready for a major release.

Sounds good, will revert that until we can change the API.

tlabaj · 2019-03-11T16:28:23Z

packages/patternfly-4/react-core/src/components/Select/Select.docs.js

@@ -1,12 +1,18 @@
-import { Select, SelectOption } from '@patternfly/react-core';
+import { Select, SelectOption, CheckboxSelectOption, CheckboxSelectGroup } from './index';


we still want o import fro @patternfly/react-core

tlabaj · 2019-03-11T16:35:14Z

packages/patternfly-4/react-core/src/components/Select/Select.js

          )}
+          {variant === 'checkbox' && (
+            <React.Fragment>
+              <SelectToggle


can this SelectToggle be made common between the two variants?

Yeah I can make it common, I was split between doing that or maintaining one switch type logic for getting the right variant.

dlabaj

Can you update the broken JEST Tests?

mcarrano · 2019-03-29T15:05:00Z

@rachael-phillips This looks good from a design and mouse interaction perspective except for the CSS issue on badge positioning. But looks like there still may be some keyboard interaction issues @jgiardino ?

also remove console statement from select

…oggle content badge

revert title name change, move group id, refactor common toggle, add grouped checkbox example

…mparison

… proptypes

jgiardino · 2019-04-01T18:52:06Z

I tested the latest updates. The keyboard interaction works as expected! Thanks!

I did notice while reviewing that there are some weird behaviors when a menu is visible, and I scroll and click another menu. The page will scroll back to the menu that was previously visible.

I'm doing one more review of the aria attributes that are defined, and there are a few that need to be modified:

aria-selected should be removed from label.pf-c-check pf-c-select__menu-item
aria-haspopup - This is just a nit, but is it possible to not include this for these components instead of setting it to false? This is not a big deal if it's significant rework.
aria-hidden="true" should be included for .pf-c-select__menu-group-title
aria-label on the <fieldset> in Checkbox Select Input
- There was previous discussion on this here and here. In looking at this now, I see that the aria-label defined on <Select> is passed to the <fieldset>, but the <fieldset> also has aria-labelledby associated with the {titleId} value provided for ariaLabelledBy on <Select>. We should only have one of these defined at a time. I'm inclined to say that if a user explicitly defines a text string in aria-label, then we should only include that attribute, and not include aria-labelledby. Is that possible?
- When I don't include ariaLabelledBy on the component, I still see aria-labelledby on the <fieldset> with no value. Is it possible to not include this attribute in those cases?
- Also, is this one of those cases where this is an html attribute defined on the react component and getting passed down to one of the html elements in the component? In cases like this, are we documenting where these attributes get added? (e.g. aside from looking at this example, how would consumers know how this attribute is getting used?)

jgiardino

Thanks for making updates! Everything looks good to me!

dlabaj · 2019-04-01T21:41:03Z

@christiemolloy @mcarrano Is this all set for UX / CSS review?

mcoker · 2019-04-01T22:02:31Z

@kmcfaul can you move the badge into the .pf-c-select__toggle-wrapper element? We made that change in patternfly/patternfly#1595, so the positioning should be fixed now.

tlabaj

LGTM

christiemolloy

LGTM, ill wait for @mcoker to add the CSS/UX tag

mcarrano · 2019-04-02T19:39:52Z

Looks good @kmcfaul Changed to 'UX Approved'

priley86 · 2019-04-15T13:00:19Z

cc: @aljesusg - do you mind giving this a look and seeing if it fits your use case? If not, maybe we can start an issue, but terrific job exploring this w/ the selector pattern in Kiali 👍

kmcfaul added the PF4 label Mar 4, 2019

kmcfaul requested review from dlabaj, jgiardino, jschuler, mcoker and tlabaj March 4, 2019 18:02

jgiardino reviewed Mar 5, 2019

View reviewed changes

tlabaj assigned dlabaj Mar 6, 2019

tlabaj requested review from christiemolloy and removed request for jschuler March 6, 2019 16:11

tlabaj added ux review A11y labels Mar 6, 2019

kmcfaul force-pushed the checkbox-select branch from 5cd2cb5 to 092e8a9 Compare March 8, 2019 00:57

kmcfaul force-pushed the checkbox-select branch from 092e8a9 to 67759d7 Compare March 11, 2019 16:39

tlabaj requested changes Mar 13, 2019

View reviewed changes

kmcfaul force-pushed the checkbox-select branch from 67759d7 to 7aaa18f Compare March 13, 2019 17:40

dlabaj requested changes Mar 29, 2019

View reviewed changes

kmcfaul added 9 commits April 1, 2019 10:45

feat(Select): add checkbox select variant

4a0667e

fix(Select): add missing ts export from rebase

1d45bf3

feat(Select): add checkbox select to TS demo

30a00fd

also remove console statement from select

feat(Select): update fieldset html, select group wrapping html, fix t…

a576d77

…oggle content badge

fix(Select): add missing form group wrapper

484ab96

feat(Select): implement pr feedback

512dfa6

revert title name change, move group id, refactor common toggle, add grouped checkbox example

feat(Select): add focus trap and tabbing, use variant constant for co…

360970b

…mparison

feat(Select): update keyboard interaction, remove internal props from…

91e84fd

… proptypes

fix(Select): update snapshot

0874943

kmcfaul force-pushed the checkbox-select branch from 36c1f78 to 0874943 Compare April 1, 2019 17:28

feat(Select): update with aria feedback

391f5c6

dlabaj previously approved these changes Apr 1, 2019

View reviewed changes

jgiardino previously approved these changes Apr 1, 2019

View reviewed changes

feat(Select): move selections badge into toggle wrapper

f10b221

kmcfaul dismissed stale reviews from jgiardino and dlabaj via f10b221 April 2, 2019 14:52

Merge branch 'master' into checkbox-select

b24fa18

tlabaj approved these changes Apr 2, 2019

View reviewed changes

christiemolloy approved these changes Apr 2, 2019

View reviewed changes

dlabaj merged commit 3b64c00 into patternfly:master Apr 2, 2019

mcarrano added ux approved and removed ux review labels Apr 2, 2019

dgutride mentioned this pull request Apr 18, 2019

PF4: Custom Select Component: Checkbox select (multiple items) variation #1217

Closed

		@@ -1,12 +1,18 @@
		import { Select, SelectOption } from '@patternfly/react-core';
		import { Select, SelectOption, CheckboxSelectOption, CheckboxSelectGroup } from './index';

feat(Select): add checkbox select variant #1487

feat(Select): add checkbox select variant #1487

Uh oh!

Conversation

kmcfaul commented Mar 4, 2019

Uh oh!

patternfly-build commented Mar 4, 2019

Uh oh!

codecov-io commented Mar 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

mcoker commented Mar 4, 2019

Uh oh!

jgiardino commented Mar 5, 2019

Uh oh!

jgiardino commented Mar 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jgiardino Mar 5, 2019

Choose a reason for hiding this comment

Uh oh!

jgiardino commented Mar 5, 2019

Uh oh!

kmcfaul commented Mar 5, 2019

Uh oh!

kmcfaul commented Mar 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jgiardino commented Mar 5, 2019

Uh oh!

mcoker commented Mar 6, 2019

Uh oh!

kmcfaul commented Mar 8, 2019

Uh oh!

mcoker commented Mar 8, 2019

Uh oh!

jgiardino commented Mar 11, 2019

Uh oh!

tlabaj Mar 11, 2019

Choose a reason for hiding this comment

Uh oh!

kmcfaul Mar 13, 2019

Choose a reason for hiding this comment

Uh oh!

tlabaj Mar 11, 2019

Choose a reason for hiding this comment

Uh oh!

tlabaj Mar 11, 2019

Choose a reason for hiding this comment

Uh oh!

kmcfaul Mar 13, 2019

Choose a reason for hiding this comment

Uh oh!

dlabaj left a comment

Choose a reason for hiding this comment

Uh oh!

mcarrano commented Mar 29, 2019

Uh oh!

jgiardino commented Apr 1, 2019

Uh oh!

jgiardino left a comment

Choose a reason for hiding this comment

Uh oh!

dlabaj commented Apr 1, 2019

Uh oh!

mcoker commented Apr 1, 2019

Uh oh!

tlabaj left a comment

Choose a reason for hiding this comment

Uh oh!

christiemolloy left a comment

Choose a reason for hiding this comment

Uh oh!

mcarrano commented Apr 2, 2019

Uh oh!

priley86 commented Apr 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

codecov-io commented Mar 4, 2019 •

edited

Loading

jgiardino commented Mar 5, 2019 •

edited

Loading

kmcfaul commented Mar 5, 2019 •

edited

Loading

priley86 commented Apr 15, 2019 •

edited

Loading