feat(select-checkbox): added filter and search input #2693

mattnolting · 2020-02-10T17:07:31Z

patternfly-build · 2020-02-10T22:03:21Z

PatternFly-Next preview: https://patternfly-next-pr-2693.surge.sh

Preview: https://patternfly-next-pr-2693.surge.sh

A11y report: https://patternfly-next-pr-2693-coverage.surge.sh

CSS Size Report

Name	Current	Previous	Diff %
components/FormControl/form-control.css	13.0 kB	11.8 kB	9.35
components/Select/select.css	17.9 kB	17.4 kB	3.11
patternfly.min.css	570.5 kB	568.8 kB	0.30
patternfly-no-reset.css	648.0 kB	646.2 kB	0.27
patternfly.css	649.8 kB	648.0 kB	0.27
patternfly-ie11.css	511.3 kB	510.6 kB	0.13

matthewcarleton

nice work! Just a few minor questions.

matthewcarleton · 2020-02-11T14:21:50Z

src/patternfly/components/Select/select.scss

@@ -443,6 +451,23 @@
  padding-top: var(--pf-c-select__menu-group--not-first--PaddingTop);
 }

+.pf-c-select__menu-group {
+  &.pf-m-padding {


Why use a modifier here for padding? Wouldn't it always need the padding? If it is a modifier should it be called padding? I didn't think we named them like this.

Yeah, we're using .pf-m-padding in some components (.pf-c-drawer most recently) where padding is a boolean and pre-configured. This is what I thought to be the most flexible approach as we build out functionality in the future opting for existing, generic elements over specific elements like .pf-c-select__filter. Wdyt?

ah gotcha so more reusable this way. Ok, I'm behind on the newest rules for PF :)

and that var is wrong, lol

src/patternfly/components/Select/select.scss

matthewcarleton · 2020-02-11T14:37:14Z

src/patternfly/components/Select/select.scss

+  --pf-c-select__menu-group--m-padding--PaddingBottom: var(--pf-c-select__menu--PaddingBottom);
+  --pf-c-select__menu-group--m-padding--PaddingLeft: var(--pf-c-select__menu-item--PaddingLeft);
+  --pf-c-select__menu-group--m-padding--not--first-of-type--PaddingTop: var(--pf-global--spacer--md);
+  --pf-c-select__menu-group--m-padding--not--last-of-type--PaddingBottom: var(--pf-global--spacer--md);


Suggested change

--pf-c-select__menu-group--m-padding--not--last-of-type--PaddingBottom: var(--pf-global--spacer--md);

--pf-c-select__menu-group--m-padding-not--last-of-type--PaddingBottom: var(--pf-global--spacer--md);

same question as above :)

Same as above

christiemolloy · 2020-02-11T18:39:20Z

src/patternfly/components/FormControl/form-control.scss

@@ -72,6 +77,13 @@ $pf-c-form-control--success--Coordinates: "M504 256c0 136.967-111.033 248-248 24
  --pf-c-form-control--success--check--Background: var(--pf-c-form-control--success--BackgroundUrl) var(--pf-c-form-control--success--BackgroundPosition) / var(--pf-c-form-control--success--BackgroundSize) no-repeat;
  --pf-c-form-control--success--Background: var(--pf-c-form-control--BackgroundColor) var(--pf-c-form-control--success--check--Background);

+  // Input m-search
+  --pf-c-form-control--m-search--PaddingLeft: var(--pf-global--spacer--xl);
+  --pf-c-form-control--m-search--BackgroundPosition: var(--pf-c-form-control--PaddingLeft);


Should the background position always align with the left padding of the form?

Yeah, because the clear (x) appears on the right by default in the input="search"

christiemolloy · 2020-02-11T18:40:33Z

src/patternfly/components/FormControl/form-control.scss

+  // Input m-search
+  --pf-c-form-control--m-search--PaddingLeft: var(--pf-global--spacer--xl);
+  --pf-c-form-control--m-search--BackgroundPosition: var(--pf-c-form-control--PaddingLeft);
+  --pf-c-form-control--m-search--BackgroundSize: var(--pf-c-form-control--FontSize) var(--pf-c-form-control--FontSize);


Should the background-size use font-size or line-height?

I copied these settings from the previous examples

christiemolloy · 2020-02-11T18:44:54Z

src/patternfly/components/Select/examples/Select.md

@@ -196,6 +203,7 @@ The checkbox select can select multiple items using checkboxes. The number of it
 | `.pf-c-select__menu-fieldset` | `<fieldset>` |  Initiates a fieldset for the items in a checkbox select. |
 | `.pf-c-select__menu-group` | `<div>` |  Initiates a group within a select menu. |
 | `.pf-c-select__menu-group-title` | `<div>` |  Initiates a title for a group with a select menu. |
+| `.pf-m-padding` | `.pf-c-select__menu-group` |  Adds padding to group. |


Change comment to something similar to this: "Modifies the select menu group to add extra padding."

src/patternfly/components/Select/select.scss

christiemolloy · 2020-02-11T18:53:44Z

src/patternfly/components/Select/select-menu-filter.hbs

@@ -0,0 +1,4 @@
+{{#> select-menu-group select-menu-group--modifier="pf-m-padding"}}


Should we follow what is in the context selector component? It has its own element wrapper for the input. The advantage of this is that it is specific for adding an input in it. While the modifier that you're applying to the pf-c-select__menu-group keeps it flexible, it may mean that users use it for the wrong reasons other than what its meant for.

Probably should do that. If in the future we want to add .pf-c-select__menu-group.pf-m-padding in the future, we can.

christiemolloy · 2020-02-12T14:24:33Z

src/patternfly/components/Select/select-menu-input.hbs

@@ -0,0 +1,4 @@
+<div class="pf-c-select__menu-input{{#if select-menu-input--modifier}} {{select-menu-input--modifier}}{{/if}}">
+  {{#> form-control controlType="input" form-control--modifier="pf-m-search" form-control--attribute=(concat 'type="search" id="' id '-search-input" name="' id '-search-input" aria-label="Search"')}}


What if in the future we want to put a different type of input into this wrapper? I guess we could leave this and then add a condition inside of here to either show search or a different type of input, wdyt?

Love this comment/suggestion. Nice!!!

christiemolloy · 2020-02-12T14:25:18Z

src/patternfly/components/Select/select.scss

+}
+
+.pf-c-select__menu-input {
+  padding-top: var(--pf-c-select__menu-input--PaddingTop);


could you just do padding: ____ ____ ____ ____ here

christiemolloy

PERFECTO

matthewcarleton

noice!

redallen · 2020-02-13T14:04:33Z

🎉 This PR is included in version 2.63.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

mattnolting requested review from christiemolloy and matthewcarleton February 10, 2020 18:50

matthewcarleton reviewed Feb 11, 2020

View reviewed changes

christiemolloy requested changes Feb 11, 2020

View reviewed changes

mattnolting added 5 commits February 11, 2020 16:50

feat(select-checkbox): added filter and search input

244501e

feat(select-checkbox): fixed a11y issue

086c509

feat(select-checkbox): fixed ally lint issue

0f81a8e

feat(select-checkbox): updated var name

e1ea878

feat(select-checkbox): changed approach to dedicated element

5b29252

christiemolloy reviewed Feb 12, 2020

View reviewed changes

mattnolting added 3 commits February 12, 2020 09:54

feat(select-checkbox): added search input and nested

24c009e

feat(select-checkbox): updated docs

f3c8654

feat(select-checkbox): ignore package-lock

eb38a1b

christiemolloy approved these changes Feb 12, 2020

View reviewed changes

matthewcarleton approved these changes Feb 13, 2020

View reviewed changes

christiemolloy merged commit c2e17a6 into patternfly:master Feb 13, 2020

redallen added the released label Feb 13, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(select-checkbox): added filter and search input #2693

feat(select-checkbox): added filter and search input #2693

mattnolting commented Feb 10, 2020

patternfly-build commented Feb 10, 2020 •

edited

Loading

matthewcarleton left a comment

matthewcarleton Feb 11, 2020

mattnolting Feb 11, 2020

matthewcarleton Feb 11, 2020

mattnolting Feb 11, 2020

matthewcarleton Feb 11, 2020

mattnolting Feb 11, 2020

christiemolloy Feb 11, 2020

mattnolting Feb 11, 2020

christiemolloy Feb 11, 2020

mattnolting Feb 11, 2020

christiemolloy Feb 11, 2020

christiemolloy Feb 11, 2020

mattnolting Feb 11, 2020

christiemolloy Feb 12, 2020

mattnolting Feb 12, 2020

christiemolloy Feb 12, 2020

christiemolloy left a comment

matthewcarleton left a comment

redallen commented Feb 13, 2020

	--pf-c-select__menu-group--m-padding--not--last-of-type--PaddingBottom: var(--pf-global--spacer--md);
	--pf-c-select__menu-group--m-padding-not--last-of-type--PaddingBottom: var(--pf-global--spacer--md);

		@@ -0,0 +1,4 @@
		{{#> select-menu-group select-menu-group--modifier="pf-m-padding"}}

		@@ -0,0 +1,4 @@
		<div class="pf-c-select__menu-input{{#if select-menu-input--modifier}} {{select-menu-input--modifier}}{{/if}}">
		{{#> form-control controlType="input" form-control--modifier="pf-m-search" form-control--attribute=(concat 'type="search" id="' id '-search-input" name="' id '-search-input" aria-label="Search"')}}

feat(select-checkbox): added filter and search input #2693

feat(select-checkbox): added filter and search input #2693

Conversation

mattnolting commented Feb 10, 2020

patternfly-build commented Feb 10, 2020 • edited Loading

matthewcarleton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

christiemolloy left a comment

Choose a reason for hiding this comment

matthewcarleton left a comment

Choose a reason for hiding this comment

redallen commented Feb 13, 2020

patternfly-build commented Feb 10, 2020 •

edited

Loading