feat(data toolbar): add data toolbar #2119

mattnolting · 2019-08-01T12:12:03Z

closes: #1528
relates to: #2041

Overflow menu is a placeholder at the moment, PR for that to come..

patternfly-build · 2019-08-01T12:16:25Z

~~Deploy preview for pf-next ready!~~

~~Built with commit 0ac72c6fe71fe5acfe47d41d2a15d8def17bc5e5~~

~~https://deploy-preview-2119--pf-next.netlify.com~~

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

dgutride · 2019-08-01T13:55:51Z

src/site/workspace.scss


+// Toolbar documentation
+.toolbar-documentation {


These will all be lost when we show the documentation on patternfly.org - please chat with me so we can discuss a different way to accomplish this. If something in the preview is deficient for showing toolbar examples - we'll need to make a global change instead.

jgiardino · 2019-08-01T19:30:11Z

Great job on this so far. This is impressive work and it's great to see this pattern starting to get implemented.

I reviewed this in person with @mattnolting, and here are some of the things we chatted about...

My original expectation when I saw the design pattern was that for the scenario where the filters are available from a toggle button, that the filters would display within something like a popover/modal type of container. This is consistent with my very black and white perspective on the world of UI, where things that display on top of the page use a modal interaction pattern, and things that display inline use the disclosure interaction pattern. But at the time of reviewing the design, I didn't realize that there are really 3 variations of this toolbar:

All controls are inline, no toggle - at the larger breakpoint
A toggle button that shows the filters group, and the group is inline
A toggle button that shows the filters group, and the group is on top of contents - at the smaller breakpoint

This raised some questions about how we really want to handle the different breakpoints, and whether html should handle all of this, or if we should instead handle the breakpoints in the react implementation given that each variation has slightly different behavior and aria semantics to implement.

We did talk about why the trapping of keyboard focus and screen reader focus would matter at smaller breakpoints. Here are a couple of scenarios where that could occur:

a screen reader user is using a phone with a bluetooth keyboard
a low vision user has zoomed in on the browser, triggering the responsive layout AND the low vision user is also a keyboard only user.
- NOTE: I don't think breakpoints are set up to use rems currently, but we should so that low vision users can take advantage of the responsive layouts for cases like this

We also talked about updating the a11y documentation to clearly identify which aria attributes are important for which layout/breakpoint size, so that it's more obvious which patterns are needed for which layout.

redallen · 2019-08-06T19:55:38Z

Merged master to get latest CircleCI changes.

patternfly-build · 2019-08-06T20:04:58Z

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

mcoker

Nice work matt!!! Left some first pass comments just looking at the changed files.

mcoker · 2019-08-13T21:18:39Z

src/patternfly/components/DataToolbar/data-toolbar-expandable-content.hbs

@@ -0,0 +1,9 @@
+<div class="pf-c-data-toolbar__expandable-content{{#if data-toolbar-expandable-content--IsExpanded}} pf-m-expanded{{/if}}{{#if data-toolbar-expandable-content--modifier}} {{data-toolbar-expandable-content--modifier}}{{/if}}" id="{{data-toolbar--id}}"


can you put id="{{data-toolbar--id}}" on a new line so it's before {{#if data-toolbar-expandable-content--attribute}}?

mcoker · 2019-08-13T21:24:23Z

src/patternfly/components/DataToolbar/data-toolbar-group-toggle-group.hbs

@@ -0,0 +1,6 @@
+<div class="pf-c-data-toolbar__group pf-m-toggle-group{{#if data-toolbar-group--modifier}} {{data-toolbar-group--modifier}}{{/if}}{{#if data-toolbar-group-toggle-group--Reveal}} pf-m-reveal{{data-toolbar-group-toggle-group--Reveal}}{{/if}}{{#if data-toolbar-toggle-group--modifier}} {{data-toolbar-toggle-group--modifier}}{{/if}}"
+  {{#if data-toolbar-toggle-group--attribute}}


should you use data-toolbar-group-toggle-group--[modifier/attribute] as the prefix for this partial's attribute vars? You're using data-toolbar-group-toggle-group--Reveal. Generally the attribute matches the partial name.

mcoker · 2019-08-13T21:25:02Z

src/patternfly/components/DataToolbar/data-toolbar-item-overflow-menu.hbs

@@ -0,0 +1,6 @@
+<div class="pf-c-data-toolbar__item pf-m-overflow-menu{{#if data-toolbar-overflow-menu--modifier}} {{data-toolbar-overflow-menu--modifier}}{{/if}}"
+  {{#if data-toolbar-overflow-menu--attribute}}


same here, should you use data-toolbar-item-overflow-menu--[attribute/modifier]?

mcoker · 2019-08-13T21:26:47Z

src/patternfly/components/DataToolbar/data-toolbar-item-search-filter.hbs

@@ -0,0 +1,11 @@
+{{#> data-toolbar-item data-toolbar-item--modifier=(concat 'pf-m-search-filter ' data-toolbar-item-search-filter--modifier)}}


Looks like you're allowing for data-toolbar-item-search-filter--modifier in this item modifier partial, but not the others (bulk select, chip group, pagination, separator). You also support data-toolbar-group-toggle-group--modifier in the toggle group modifier partial. Do you think we should add it to the others for consistency?

mcoker · 2019-08-13T21:29:08Z

src/patternfly/components/DataToolbar/data-toolbar-group-toggle-group.hbs

@@ -0,0 +1,6 @@
+<div class="pf-c-data-toolbar__group pf-m-toggle-group{{#if data-toolbar-group--modifier}} {{data-toolbar-group--modifier}}{{/if}}{{#if data-toolbar-group-toggle-group--Reveal}} pf-m-reveal{{data-toolbar-group-toggle-group--Reveal}}{{/if}}{{#if data-toolbar-toggle-group--modifier}} {{data-toolbar-toggle-group--modifier}}{{/if}}"


Should you use something like {{#> data-toolbar-group data-toolbar-group--modifier=(concat 'pf-m-toggle-group ' data-toolbar-toggle-group--modifier)}}

And do you need {{#if data-toolbar-group--modifier}} {{data-toolbar-group--modifier}}{{/if}} here? Or could modifiers to this partial be covered by data-toolbar-toggle-group--modifier?

mcoker · 2019-08-13T21:36:02Z

src/patternfly/components/DataToolbar/data-toolbar.scss

+  // Toggle group
+  --pf-c-data-toolbar__group--m-toggle-group--spacer: var(--pf-global--spacer--sm);
+  --pf-c-data-toolbar__group--m-toggle-group--m-reveal--spacer: var(--pf-c-data-toolbar--spacer--base);
+  --pf-c-data-toolbar__toggle--c-badge: var(--pf-global--spacer--sm);


looks like this should be --pf-c-data-toolbar__toggle--c-badge--MarginLeft

mcoker · 2019-08-13T21:37:06Z

src/patternfly/components/DataToolbar/data-toolbar.scss

+  --pf-c-data-toolbar--m-search-filter--spacer: var(--pf-global--spacer--sm);
+  --pf-c-data-toolbar--m-search-filter--space-items: 0;
+
+  // Separator


Can we use the divider component here?

I don't think so, we don't have a vertical divider option yet...

Oh sorry, I meant adding that as an enhancement to that component and using it here. We didn't create the vertical divider because we didn't need that variation yet, but I think we do now. I'll create an issue for that and we can use this as it is and update it once we add the variation to the divider.

mcoker · 2019-08-13T21:41:40Z

src/patternfly/components/DataToolbar/data-toolbar.scss

+  &.pf-m-expanded {
+    display: grid;
+    grid-template-columns: 1fr;
+    grid-row-gap: var(--pf-c-data-toolbar__expandable-content--m-expanded--GridGap);


Can you use GridRowGap in the var?

mcoker · 2019-08-13T21:44:18Z

src/patternfly/components/DataToolbar/examples/index.js

+  const dataToolbarGroupSpacersExample = DataToolbarGroupSpacersExample();
+  const dataToolbarStackedExample = DataToolbarStackedExample();
+
+  const headingText = 'Data Toolbar';


Suggested change

const headingText = 'Data Toolbar';

const headingText = 'Data toolbar';

mcoker · 2019-08-13T21:47:53Z

src/patternfly/components/OptionsMenu/options-menu.scss

@@ -155,6 +155,8 @@
  }

  &.pf-m-plain {
+    justify-content: center;


This declaration is in the selector below that excludes the .pf-m-text variation. If it applies to the .pf-m-text variation and you set a width on the options menu, you get this:

mcoker · 2019-08-13T22:15:19Z

src/patternfly/components/DataToolbar/data-toolbar.scss

+
+  // Spacer values
+  --pf-c-data-toolbar__item--spacer: var(--pf-c-data-toolbar--spacer--base);
+  --pf-c-data-toolbar__group--spacer: var(--pf-c-data-toolbar--spacer--base);


is it no longer the case that standalone items and/or groups have a 24px spacer and that items in a group have 16px?

That is correct, @mceledonia and I reviewed the spec and arrived at this conclusion.

mcoker · 2019-08-13T22:16:17Z

src/patternfly/components/DataToolbar/docs/code.md

+| Class | Applied to | Outcome |
+| -- | -- | -- |
+| `.pf-c-data-toolbar` | `<div>` | Initiates the toolbar component. **Required** |
+| `.pf-c-data-toolbar__item` | `<div>` | Initiates the toolbar component item. **Required** |


I would use "Initiates a toolbar component item" since you can have more than 1 of them. And continue to use "the" when you can only have 1.

mcoker · 2019-08-13T22:18:19Z

src/patternfly/components/DataToolbar/docs/code.md

+| `.pf-c-data-toolbar__expandable-content` | `<div>` | Initiates the toolbar component expandable content. |
+| `.pf-m-expanded` | `.pf-c-data-toolbar__expandable-content` | Modifies the component for the expanded state. |
+| `.pf-c-data-toolbar__divider` | `<div>` | Initiates the toolbar component divider. |
+| `.pf-m-bulk-select` | `.pf-c-data-toolbar__item` | Modifies toolbar item spacing. |


Usually we'll put a description of what it modifies, like "modifies bulk select toolbar item spacing"

mcoker · 2019-08-13T22:19:02Z

src/patternfly/components/DataToolbar/docs/code.md

+| `.pf-m-icon-button-group` | `.pf-c-data-toolbar__group` | Modifies toolbar group spacing. |
+| `.pf-m-filter-group` | `.pf-c-data-toolbar__group` | Modifies toolbar group spacing. |
+| `.pf-m-contextual-search` | `.pf-c-data-toolbar__group` | Modifies toolbar group spacing. |
+| `.pf-m-hidden{-on-[breakpoint]}` | `.pf-c-data-toolbar > *` | Modifies toolbar element to hidden at breakpoint. |


Instead of "to hidden" made "to hide" or "to be hidden"?

mcoker · 2019-08-14T19:09:16Z

src/patternfly/components/DataToolbar/data-toolbar.scss

+.pf-c-data-toolbar__item {
+  margin-right: var(--pf-c-data-toolbar--spacer);
+
+  &:last-child:not([class*="pf-m-spacer"]) {


nice! probably best to allow them to define this spacing even if it's the last child.

src/patternfly/components/DataToolbar/docs/data-toolbar-spacers-intro.md

mcoker · 2019-08-14T19:48:11Z

src/patternfly/components/DataToolbar/docs/data-toolbar-toggle-group-intro.md

@@ -0,0 +1,3 @@
+### Toggle group modifier
+The `.pf-m-toggle-group` controls when, and at which breakpoint, filters will be hidden and revealed. By default, all filters are hidden until the specified breakpoint is reached.
+- `.pf-m-reveal-on-{md, lg, xl}`: controls when filters are revealed and when toggle button is hidden.


bullet isn't showing up, this example doesn't have className="is-layout-page". Some others are missing it, too, not sure if it should be added to those.

mcoker · 2019-08-14T19:48:57Z

src/patternfly/components/DataToolbar/docs/data-toolbar-toggle-group.md

+| `hidden` | `.pf-c-data-toolbar__item`, `.pf-c-data-toolbar__group`, `.pf-c-data-toolbar__toggle`, `.pf-c-data-toolbar__expandable-content` |  Indicates that the toggle group element is hidden. **Required** |
+| `aria-expanded="true"` | `.pf-c-data-toolbar__toggle > .pf-c-button` |  Indicates that the expandable content is visible. **Required** |
+| `aria-expanded="false"` | `.pf-c-data-toolbar__toggle > .pf-c-button` |  Indicates the the expandable content is hidden. **Required** |
+| `aria-controls="[id of expandable content]"` | `.pf-c-data-toolbar__toggle > .pf-c-button` |  Identifies the expanded content controlled by the toggle button when . **Required** |


seems like part of the sentence is missing.

mcoker · 2019-08-14T21:11:56Z

src/patternfly/components/DataToolbar/data-toolbar.scss

+  --pf-c-data-toolbar__content--md--PaddingRight: var(--pf-global--spacer--md);
+  --pf-c-data-toolbar__content--md--PaddingLeft: var(--pf-global--spacer--md);
+
+  @media screen and (max-width: $pf-global--breakpoint--md) {


Can we use min-width here?

mcoker · 2019-08-14T21:26:46Z

src/patternfly/components/DataToolbar/data-toolbar.scss

+  --pf-c-data-toolbar--BackgroundColor: var(--pf-global--BackgroundColor--100);
+
+  // Content
+  --pf-c-data-toolbar__content--PaddingTop: var(--pf-global--spacer--md);


looks like all of the examples use the toolbar__content element. From the original issue:

There should be no spacers on the left and right edges of the toolbar if there are already padding spacers being used with whatever component the toolbar is placed within (e.g. cards).

Did that requirement change?

Nope! Good catch.

mcoker · 2019-08-14T21:37:02Z

src/patternfly/components/DataToolbar/docs/data-toolbar-intro.md

+| `.pf-m-overflow-menu` | `.pf-c-data-toolbar__item` | Modifies toolbar item spacing. Spacer value is set to `var(--pf-c-data-toolbar__item--m-overflow-menu--spacer)`. |
+| `.pf-m-pagination` | `.pf-c-data-toolbar__item` | Modifies toolbar item spacing. Spacer value is set to `var(--pf-c-data-toolbar__item--m-pagination--spacer)`. |
+<br>
+<h3 class="Example_heading">Spacer system</h3>


The indentation on this gets a little wonky in markdown. I replaced this through the "available breakpoints" line with this code and it looks better

<h3 class="Example_heading">Spacer system</h3> **Spacer** - `.pf-m-spacer-{none, sm, md, lg}`. - This modifier can be applied to a group or item. - This modifier changes the spacer value for only the element to which it is applied. - Responsive spacers can be used by appending `{-on-[breakpoint]}` to `.pf-m-spacer-{size}`. **Example:** `.pf-m-spacer-lg-on-xl`. <br> **Space items** - `.pf-m-space-items-{none, sm, md, lg}`. - This modifier affects only groups. - It changes the spacing of direct child items and can be overwritten with any other modifier. - Responsive spacers can be used by appending `{-on-[breakpoint]}` to `.pf-m-space-items-{size}`. **Example:** `.pf-m-space-items-lg-on-xl`. <br> ### Available breakpoints are: `-on-md, -on-lg, -on-xl, -on-2xl`.

mcoker · 2019-08-14T21:40:13Z

src/patternfly/components/DataToolbar/data-toolbar.scss

+
+  // Icon group spacer
+  --pf-c-data-toolbar__group--m-filter-group--spacer: var(--pf-c-data-toolbar--spacer--base);
+  --pf-c-data-toolbar__group--m-filter-group--space-items: 0;


Have you considered applying spacing similar to the input group here so that you don't end up with double borders where elements meet adjacent elements?

mcoker · 2019-08-14T21:41:16Z

src/patternfly/components/DataToolbar/data-toolbar.scss

+  --pf-c-data-toolbar__group--m-icon-button-group--space-items: 0;
+
+  // Icon group spacer
+  --pf-c-data-toolbar__group--m-filter-group--spacer: var(--pf-c-data-toolbar--spacer--base);


Do you think the -group on the group modifiers is redundant? I think it's fine, but think it would be fine to drop it, too. Just wanted to mention it if you hadn't considered it.

Yeah, I thought about omitting it. -group was more readable to me than:

.pf-c-data-toolbar__group.pf-m-filters

.pf-c-data-toolbar__group.pf-m-buttons

.pf-c-data-toolbar__group.pf-m-icon-buttons

I could go either way, though

mcoker · 2019-08-14T21:53:12Z

src/patternfly/components/DataToolbar/data-toolbar.scss

+
+  &.pf-m-expanded {
+    display: grid;
+    grid-template-columns: 1fr;


do you need to define this?

nice catch. I had it there to place clear all filters button to [auto]column: 2, but I don't think we need it.

mcoker · 2019-08-15T18:27:22Z

src/patternfly/components/DataToolbar/docs/data-toolbar-intro.md

@@ -0,0 +1,46 @@
+Data toolbar relies on groups (`.pf-c-data-toolbar__group`) and items (`.pf-c-data-toolbar__item`), with default spacer values. Groups and items can be siblings and/or items can be nested within groups. Modifier selectors adjust spacing based on the type of group or item. Each modifier applies a unique CSS variable, therefore, the base spacer value for all elements can be customized and item/groups spacers can be themed individually. The default spacer value for items and groups is set to `--pf-global--spacer--md` or 16px.


do you think it's worth saying the default spacer value is set to --pf-c-data-toolbar--spacer--base, in the context of customizing/theming items/groups? And then you could mention that the value for that is --pf-global--spacer--md or 16px. Like

The default spacer value for items and groups is set to --pf-c-data-toolbar--spacer--base, whose value is --pf-global--spacer--md or 16px.

mcoker · 2019-08-15T18:32:50Z

src/patternfly/components/DataToolbar/docs/data-toolbar-intro.md

+- This modifier affects only groups.
+- It changes the spacing of direct child items and can be overwritten with any other modifier.
+- Responsive spacers can be used by appending `{-on-[breakpoint]}` to `.pf-m-space-items-{size}`. **Example:** `.pf-m-space-items-lg-on-xl`.
+


Could this also not be a standard usage table? Like

Class Applied to Outcome

.pf-m-spacer-{none, sm, md, lg}{-on-[breakpoint]} .pf-c-data-toolbar__item, .pf-c-data-toolbar__group Modifies the spacer value for only the element to which it is applied. Example: .pf-m-spacer-lg-on-xl.

.pf-m-space-items-{none, sm, md, lg}{-on-[breakpoint]} .pf-c-data-toolbar__group Modifies the spacing of direct child items and can be overwritten with any other modifier. Example: .pf-m-space-items-lg-on-xl.

er actually, this table basically exists under "adjusting item spacers" and "adjusting group spacers." Do you think it's best to document it in the intro and under those sections?

Removed from intro section

mcoker · 2019-08-15T18:36:22Z

src/patternfly/components/DataToolbar/docs/data-toolbar-toggle-group-intro.md

@@ -0,0 +1,2 @@
+### Toggle group modifier
+The `.pf-m-toggle-group` controls when, and at which breakpoint, filters will be hidden and revealed. By default, all filters are hidden until the specified breakpoint is reached. `.pf-m-reveal-on-{md, lg, xl}`: controls when filters are revealed and when toggle button is hidden.


Suggested change

The `.pf-m-toggle-group` controls when, and at which breakpoint, filters will be hidden and revealed. By default, all filters are hidden until the specified breakpoint is reached. `.pf-m-reveal-on-{md, lg, xl}`: controls when filters are revealed and when toggle button is hidden.

The `.pf-m-toggle-group` controls when, and at which breakpoint, filters will be hidden and revealed. By default, all filters are hidden until the specified breakpoint is reached. `.pf-m-reveal-on-{md, lg, xl}` controls when filters are revealed and when the toggle button is hidden.

mcoker · 2019-08-15T18:38:41Z

src/patternfly/components/DataToolbar/docs/code.md

+| `.pf-c-data-toolbar` | `<div>` | Initiates the toolbar component. **Required** |
+| `.pf-c-data-toolbar__item` | `<div>` | Initiates a the toolbar item. **Required** |
+| `.pf-c-data-toolbar__group` | `<div>` | Initiates a toolbar group. |
+| `.pf-c-data-toolbar__content` | `<div>` | Initiates a toolbar content section. |


is this required?

mcoker · 2019-08-15T18:39:54Z

src/patternfly/components/DataToolbar/docs/code.md

+| `.pf-c-data-toolbar__content` | `<div>` | Initiates a toolbar content section. |
+| `.pf-c-data-toolbar__expandable-content` | `<div>` | Initiates a toolbar expandable content section. |
+| `.pf-m-expanded` | `.pf-c-data-toolbar__expandable-content` | Modifies expandable content for the expanded state. |
+| `.pf-c-data-toolbar__divider` | `<div>` | Initiates the toolbar divider. |


can you remove this?

mcoker · 2019-08-15T19:48:51Z

src/patternfly/components/DataToolbar/data-toolbar.scss

+    margin-left: 0;
+  }
+
+  .pf-c-chip-group {


Should we have a modifier for the chip group item?

mcoker · 2019-08-15T20:16:11Z

src/patternfly/components/DataToolbar/data-toolbar.scss

+    }
+
+    > * + * {
+      margin-left: -1px;


sweeet. The only issue with this (and in the input group) is if you focus on an element that's to the left of another element in the group, the focus ring is hidden behind the right element, since the right element overlaps the left and has a higher stacking order since it comes after in the DOM.

Maybe we could use :focus-within? Here's a codepen as an example - https://codepen.io/mcoker/pen/BaBKORg. I tried it in the toolbar and needed to add a z-index, too, but didn't look at it much further as to why. Not sure that we want to add a z-index, which could cause random issues with that element popping out from underneath other elements.

:focus-within didn't work well for .pf-c-input-group text input. Setting :focus on __item children worked. My main concern w/setting z-index was related to dropdowns within the same toolbar. Setting the z-index value to var(--pf-global--ZIndex--xs) ensures that a :focused toolbar element won't appear above a dropdown. Other z-indexed elements have higher z-indexes, but it could still see it causing issues..

mcoker · 2019-08-16T18:32:24Z

src/patternfly/components/DataToolbar/data-toolbar.scss

+
+  :focus {
+    position: relative;
+    z-index: var(--pf-global--ZIndex--xs);


need a component var for this one.

mcoker · 2019-08-16T18:36:22Z

src/patternfly/components/DataToolbar/data-toolbar.scss

+  --pf-c-data-toolbar__expandable-content--lg--PaddingRight: 0;
+  --pf-c-data-toolbar__expandable-content--lg--PaddingBottom: 0;
+  --pf-c-data-toolbar__expandable-content--lg--PaddingLeft: 0;
+  --pf-c-data-toolbar__expandable-content--ZIndex: var(--pf-global--ZIndex--xs);


Looking at @christiemolloy's zindex refactor, this component should have a z-index of --pf-global--ZIndex--xs. That said, we're going to end up with situations like this - I just added an expanded dropdown after the expanded toolbar in the DOM. I think this is a likely scenario in a table or datalist with an open kebab menu in the first row on mobile.

I'm not sure how this behaves in react, but I think we should consider closing open menus when you open another menu. I know this happens with dropdowns or selects on the page, but not sure what happens if you have a mix of things like dropdowns, selects, toolbar, options menu, etc. WDYT?

@mcoker Valid point, for sure. The React dropdown closes itself with tab exit, but not with VO exit, even when another menu is opened. I think 100% agree, open menus should close when another menu is opened.

mcoker

great job @mattnolting!! 🍻

matthewcarleton

hey Matt, looks really good to me! Very impressive :)
I've left some general questions/comments.

matthewcarleton · 2019-08-19T17:47:15Z

src/patternfly/components/DataToolbar/docs/code.md

+| Class | Applied to | Outcome |
+| -- | -- | -- |
+| `.pf-c-data-toolbar` | `<div>` | Initiates the toolbar component. **Required** |
+| `.pf-c-data-toolbar__item` | `<div>` | Initiates a the toolbar item. **Required** |


matthewcarleton · 2019-08-19T17:48:21Z

src/patternfly/components/DataToolbar/docs/code.md

+| `.pf-c-data-toolbar__group` | `<div>` | Initiates a toolbar group. |
+| `.pf-c-data-toolbar__content` | `<div>` | Initiates a toolbar content section. **Required** |
+| `.pf-c-data-toolbar__expandable-content` | `<div>` | Initiates a toolbar expandable content section. |
+| `.pf-m-expanded` | `.pf-c-data-toolbar__expandable-content` | Modifies expandable content for the expanded state. |


matthewcarleton · 2019-08-19T17:49:13Z

src/patternfly/components/DataToolbar/docs/code.md

+| `.pf-c-data-toolbar__content` | `<div>` | Initiates a toolbar content section. **Required** |
+| `.pf-c-data-toolbar__expandable-content` | `<div>` | Initiates a toolbar expandable content section. |
+| `.pf-m-expanded` | `.pf-c-data-toolbar__expandable-content` | Modifies expandable content for the expanded state. |
+| `.pf-m-separator` | `.pf-c-data-toolbar__item` | Modifies separator border. |


matthewcarleton · 2019-08-19T17:49:50Z

src/patternfly/components/DataToolbar/docs/code.md

+| `.pf-c-data-toolbar__expandable-content` | `<div>` | Initiates a toolbar expandable content section. |
+| `.pf-m-expanded` | `.pf-c-data-toolbar__expandable-content` | Modifies expandable content for the expanded state. |
+| `.pf-m-separator` | `.pf-c-data-toolbar__item` | Modifies separator border. |
+| `.pf-m-bulk-select` | `.pf-c-data-toolbar__item` | Modifies bulk select spacing. |


matthewcarleton · 2019-08-19T17:49:59Z

src/patternfly/components/DataToolbar/docs/code.md

+| `.pf-m-expanded` | `.pf-c-data-toolbar__expandable-content` | Modifies expandable content for the expanded state. |
+| `.pf-m-separator` | `.pf-c-data-toolbar__item` | Modifies separator border. |
+| `.pf-m-bulk-select` | `.pf-c-data-toolbar__item` | Modifies bulk select spacing. |
+| `.pf-m-overflow-menu` | `.pf-c-data-toolbar__item` | Modifies overflow menu spacing. |


matthewcarleton · 2019-08-19T18:40:49Z

src/patternfly/components/DataToolbar/examples/data-toolbar-selected-filters-example.hbs

+    {{> data-toolbar-item-chip-group chip-group--label="Risk" chip-group--id=(concat data-toolbar--id '-group2-')}}
+
+    {{!-- Clear all filters button --}}
+    {{#> data-toolbar-item data-toolbar-item--modifier="pf-m-clear"}}


general question about pf-m-clear. For smaller screens should this still be justify-self:end when looking at this in the workspace it feels odd to have it way over on the right. Possibly better a11y if it's justify-self: start for smaller screens?

Consulted @mceledonia, he says align it right. The button looks misaligned when aligned left. @kybaker @mcarrano wyt?

ya I saw that to b/c of the spacing around the button. I wonder if this is showing a larger issue of the type of button. I'd guess it will come up again, it feels like a problem with the button itself if we are trying to hide it by aligning it left, but I understand that for the target area we might be stuck with it.

matthewcarleton · 2019-08-19T18:51:00Z

src/patternfly/components/DataToolbar/data-toolbar.scss

+  --pf-c-data-toolbar__item--child--focus--ZIndex: var(--pf-global--ZIndex--xs);
+
+  // Item
+  --pf-c-data-toolbar--item--Display: flex;


Is this needed? Doesn't look to be used.

matthewcarleton · 2019-08-19T18:51:47Z

src/patternfly/components/DataToolbar/data-toolbar.scss

+
+  // Item
+  --pf-c-data-toolbar--item--Display: flex;
+  --pf-c-data-toolbar--item--Visibility: visible;


Same for this, not seeing it used anywhere.

matthewcarleton · 2019-08-19T18:52:48Z

src/patternfly/components/DataToolbar/data-toolbar.scss

+
+  // Toggle group
+  --pf-c-data-toolbar__group--m-toggle-group--spacer: var(--pf-global--spacer--sm);
+  --pf-c-data-toolbar__group--m-toggle-group--m-reveal--spacer: var(--pf-c-data-toolbar--spacer--base);


not seeing this used anywhere.

matthewcarleton · 2019-08-19T18:53:50Z

src/patternfly/components/DataToolbar/data-toolbar.scss

+  --pf-c-data-toolbar__group--m-filter-group--space-items: 0;
+
+  // Overflow menu item
+  --pf-c-data-toolbar--m-overflow-menu--spacer: var(--pf-c-data-toolbar--spacer--base);


Yeah, the spacing is a little different when revealed

mattnolting · 2019-08-20T17:21:24Z

@matthewcarleton updated

matthewcarleton

LGTM! I still wonder about the right aligned button as I commented on but maybe that's an issue that can be addressed outside this PR.

mcoker · 2019-08-22T14:23:35Z

src/patternfly/components/DataToolbar/data-toolbar.scss

+  --pf-c-data-toolbar--m-separator--spacer: var(--pf-c-data-toolbar__item--spacer);
+  --pf-c-data-toolbar--m-separator--BackgroundColor: var(--pf-global--BorderColor--100);
+  --pf-c-data-toolbar--m-separator--Width: var(--pf-global--BorderWidth--md);
+  --pf-c-data-toolbar--m-separator--Height: var(--pf-global--FontSize--lg);


This doesn't really follow the var naming convention, except that it follows the selector order in the SCSS. Though ideally in the SCSS, because of how we write BEM modifiers, the modifier should be combined with the selector it applies to, otherwise (since we share modifier class names, and the class name doesn't include the element it applies to) we run into potential conflicts with .pf-c-data-toolbar .pf-m-separator {} applying to multiple elements that share the modifier class name (items, groups, and any other child of the toolbar - dropdowns, etc - that may use that class). And writing the var this way opens us up to conflicting names where, as an example, if we were to add .pf-m-separator to .pf-c-data-toolbar, the variable name (--pf-c-data-toolbar--m-separator) is already taken.

In a perfect world, the .pf-m-separator selector would be combined with the element it modifies, so .pf-c-toolbar__item.pf-m-separator, then the var name would be --pf-c-data-toolbar__item--m-separator. What do you think?

@mcoker updated, how's that look?

Perfect, thanks!

mattnolting · 2019-08-22T16:27:22Z

@mcoker This should be more inline with naming convention and compound selector config.

mcoker · 2019-08-22T21:24:30Z

src/patternfly/components/DataToolbar/data-toolbar.scss

-
-// Group
+// __group and __item spacer values have to be defined after base styling and modifiers
+/* stylelint-disable no-duplicate-selectors */


Why is this the case? I'm probably missing it, but I don't see why you couldn't move these up into their respective selectors above.

mcoker · 2019-08-22T21:25:51Z

src/patternfly/components/DataToolbar/data-toolbar.scss

 .pf-c-data-toolbar__item {
  // reset spacer
  --pf-c-data-toolbar--spacer: var(--pf-c-data-toolbar__item--spacer);

+  margin-right: var(--pf-c-data-toolbar--spacer);


This is duplicated in the selector that follows it.

mcoker · 2019-08-22T21:32:55Z

On https://github.com/patternfly/patternfly-next/blob/1f46b717a5c365659997bf79e112a47799adf6a8/src/patternfly/components/DataToolbar/data-toolbar.scss#L216 I think that :focus selector is going to cause problems. Here's an example where setting position: relative on any focused child will mess up positioning https://codepen.io/mcoker/pen/PoYWpPj

mcoker · 2019-08-22T21:37:17Z

src/patternfly/components/DataToolbar/data-toolbar.scss

-  --pf-c-data-toolbar--m-button-group--spacer: var(--pf-c-data-toolbar__group--spacer);
-  --pf-c-data-toolbar--m-button-group--space-items: var(--pf-global--spacer--sm);
+  --pf-c-data-toolbar__group--m-button-group--spacer: var(--pf-c-data-toolbar__group--spacer);
+  --pf-c-data-toolbar__group--m-button-group--space-items: var(--pf-global--spacer--sm);

  // Icon group spacer


Should be "// Filter group spacer"

mattnolting · 2019-08-23T12:55:07Z

I think that :focus selector is going to cause problems. Here's an example where setting position: relative on any focused child will mess up positioning

@mcoker Let's revisit this and fix the problem globally. Created a new issue here: #2184

mcoker

Great job on this @mattnolting!!! 🍻

redallen · 2019-08-23T14:18:52Z

🎉 This PR is included in version 2.27.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

mattnolting requested review from christiemolloy, mcoker, jgiardino and matthewcarleton August 1, 2019 12:12

dgutride reviewed Aug 1, 2019

View reviewed changes

mcoker reviewed Aug 13, 2019

View reviewed changes

mcoker mentioned this pull request Aug 14, 2019

Add vertical variation to divider component #2148

Closed

mcoker reviewed Aug 14, 2019

View reviewed changes

src/patternfly/components/DataToolbar/docs/data-toolbar-spacers-intro.md Outdated Show resolved Hide resolved

mcoker reviewed Aug 14, 2019

View reviewed changes

mcoker reviewed Aug 15, 2019

View reviewed changes

mcoker reviewed Aug 16, 2019

View reviewed changes

mcoker approved these changes Aug 16, 2019

View reviewed changes

matthewcarleton reviewed Aug 19, 2019

View reviewed changes

matthewcarleton approved these changes Aug 22, 2019

View reviewed changes

matthewcarleton mentioned this pull request Aug 22, 2019

Button alignment in data toolbar #2179

Closed

mcoker reviewed Aug 22, 2019

View reviewed changes

refactor(toolbar): add data-toolbar

804ee4e

mcoker approved these changes Aug 23, 2019

View reviewed changes

mcoker merged commit a4a8fa8 into patternfly:master Aug 23, 2019

redallen added the released label Aug 23, 2019

		@@ -0,0 +1,9 @@
		<div class="pf-c-data-toolbar__expandable-content{{#if data-toolbar-expandable-content--IsExpanded}} pf-m-expanded{{/if}}{{#if data-toolbar-expandable-content--modifier}} {{data-toolbar-expandable-content--modifier}}{{/if}}" id="{{data-toolbar--id}}"

		@@ -0,0 +1,6 @@
		<div class="pf-c-data-toolbar__group pf-m-toggle-group{{#if data-toolbar-group--modifier}} {{data-toolbar-group--modifier}}{{/if}}{{#if data-toolbar-group-toggle-group--Reveal}} pf-m-reveal{{data-toolbar-group-toggle-group--Reveal}}{{/if}}{{#if data-toolbar-toggle-group--modifier}} {{data-toolbar-toggle-group--modifier}}{{/if}}"
		{{#if data-toolbar-toggle-group--attribute}}

		@@ -0,0 +1,6 @@
		<div class="pf-c-data-toolbar__item pf-m-overflow-menu{{#if data-toolbar-overflow-menu--modifier}} {{data-toolbar-overflow-menu--modifier}}{{/if}}"
		{{#if data-toolbar-overflow-menu--attribute}}

		@@ -0,0 +1,11 @@
		{{#> data-toolbar-item data-toolbar-item--modifier=(concat 'pf-m-search-filter ' data-toolbar-item-search-filter--modifier)}}

	const headingText = 'Data Toolbar';
	const headingText = 'Data toolbar';

		@@ -0,0 +1,46 @@
		Data toolbar relies on groups (`.pf-c-data-toolbar__group`) and items (`.pf-c-data-toolbar__item`), with default spacer values. Groups and items can be siblings and/or items can be nested within groups. Modifier selectors adjust spacing based on the type of group or item. Each modifier applies a unique CSS variable, therefore, the base spacer value for all elements can be customized and item/groups spacers can be themed individually. The default spacer value for items and groups is set to `--pf-global--spacer--md` or 16px.

Class	Applied to	Outcome
`.pf-m-spacer-{none, sm, md, lg}{-on-[breakpoint]}`	`.pf-c-data-toolbar__item`, `.pf-c-data-toolbar__group`	Modifies the spacer value for only the element to which it is applied. Example: `.pf-m-spacer-lg-on-xl`.
`.pf-m-space-items-{none, sm, md, lg}{-on-[breakpoint]}`	`.pf-c-data-toolbar__group`	Modifies the spacing of direct child items and can be overwritten with any other modifier. Example: `.pf-m-space-items-lg-on-xl`.

		@@ -0,0 +1,2 @@
		### Toggle group modifier
		The `.pf-m-toggle-group` controls when, and at which breakpoint, filters will be hidden and revealed. By default, all filters are hidden until the specified breakpoint is reached. `.pf-m-reveal-on-{md, lg, xl}`: controls when filters are revealed and when toggle button is hidden.

	\| `.pf-c-data-toolbar__item` \| `<div>` \| Initiates a the toolbar item. Required \|
	\| `.pf-c-data-toolbar__item` \| `<div>` \| Initiates a toolbar item. Required \|

	\| `.pf-m-expanded` \| `.pf-c-data-toolbar__expandable-content` \| Modifies expandable content for the expanded state. \|
	\| `.pf-m-expanded` \| `.pf-c-data-toolbar__expandable-content` \| Modifies expandable content section for the expanded state. \|

	\| `.pf-m-separator` \| `.pf-c-data-toolbar__item` \| Modifies separator border. \|
	\| `.pf-m-separator` \| `.pf-c-data-toolbar__item` \| Initiates separator border. \|

	\| `.pf-m-bulk-select` \| `.pf-c-data-toolbar__item` \| Modifies bulk select spacing. \|
	\| `.pf-m-bulk-select` \| `.pf-c-data-toolbar__item` \| Initiates bulk select spacing. \|

	\| `.pf-m-overflow-menu` \| `.pf-c-data-toolbar__item` \| Modifies overflow menu spacing. \|
	\| `.pf-m-overflow-menu` \| `.pf-c-data-toolbar__item` \| Initiates overflow menu spacing. \|

feat(data toolbar): add data toolbar #2119

feat(data toolbar): add data toolbar #2119

Conversation

mattnolting commented Aug 1, 2019 • edited

patternfly-build commented Aug 1, 2019 • edited by mattnolting

Choose a reason for hiding this comment

jgiardino commented Aug 1, 2019

redallen commented Aug 6, 2019

patternfly-build commented Aug 6, 2019

mcoker 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

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

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

mattnolting Aug 16, 2019 • edited

Choose a reason for hiding this comment

mcoker left a comment

Choose a reason for hiding this comment

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

mattnolting commented Aug 20, 2019

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

mattnolting commented Aug 1, 2019 •

edited

patternfly-build commented Aug 1, 2019 •

edited by mattnolting

mattnolting Aug 16, 2019 •

edited