feat(button): add input modifier by christiemolloy · Pull Request #2005 · patternfly/patternfly · GitHub

closes #1926

As per @mcoker suggestion, added a new modifier to the button component that styles it for use in with an input.

Also updated the clipboard copy component, context selector component and input component to now use this button.

Deploy preview for pf-next ready!

Built with commit ad6c56c

https://deploy-preview-2005--pf-next.netlify.com

@mcoker @mattnolting I removed the hbs file for pf-c-clipboard-copy__group-copy because it was only just a wrapper. I didn't remove the hbs for pf-c-clipboard-copy__group-toggle because its used to determine whether its expanded or not, what do you both think about that?

Would we also want to use this variation of the button as the dropdown (and components like it) toggle with left/right padding changes?

I wonder if we should add a modifier to the button for this, like pf-m-expanded/selected/active or something like that, since I imagine we will want to apply these styles in other contexts, too?

My take was that the button should just hold the generic hover, active and focus styles, and then components that use the button can add their own modifiers since they're specific to that component, and we can't forsee all of the modifier names. But what is your main reasoning for adding these modifiers to the button I'm open to adding them.

The name is the tricky part, but it's a common form control display for active/focus - the thicker, blue bottom border. We would also use it for expanded as the dropdown toggle, too, if we use it there.

So are you suggesting adding pf-m-expanded to the button since we know we're going to be using that in the clipboard copy at least.

  --pf-c-button--m-input--m-expanded--BorderBottomWidth: var(--pf-global--BorderWidth--md);
  --pf-c-button--m-input--m-expanded--BorderBottomColor: var(--pf-global--active-color--100);

pf-m-selected/active: not sure about these classes because I think the active/focus styles cover these, or am I missing something?

Yep, that's it. We can always not since it's only used here for now, and add it later if we end up using it elsewhere (like the dropdown or other components). It just looks like a state this type of button would need.

Also what's the reasoning behind pf-m-input as the modifier name? Is it because the intended use is always going to be "for use in with an input." (as in connected to an input?) I wonder if that's going to end up being limiting.

Added the expanded modifier to the button.. @mceledonia do you have any ideas for a name for this button?

@christiemolloy Is this for the overall modifier to change styling to input group? If so, maybe something like input-group-button or something specific like that. Maybe something that rolls off the tongue a bit better :)

Its for these buttons, thanks for the suggestion!

@mceledonia on second thought, if we use this button in the individual dropdown/select etc. then i dont know if input-group-button would be correct, since it wouldn't be part of an input. Maybe a name that refers to the style more so the usage?

As a side note here, I think changing to 2px/blue on hover looks better than the current black to blue change, which isn't very noticeable. Then in the case of the dropdown arrow, leaving the 2px blue style active. If we made this change it would apply to all buttons/inputs/dropdowns that use this style, something to consider.

Should the :hover weight be 2px ^^?

Is there a reason we don't just add the group-toggle class and all of the attributes to the button, like we did with the group-copy button?

I added a comment here about it: #2005 (comment)
but i think its possible to give it a try

As a side note here, I think changing to 2px/blue on hover looks better than the current black to blue change, which isn't very noticeable. Then in the case of the dropdown arrow, leaving the 2px blue style active. If we made this change it would apply to all buttons/inputs/dropdowns that use this style, something to consider.

Should the :hover weight be 2px ^^?

@mattnolting yep I added a variable for the hover weight: --pf-c-button--m-input--hover--BorderBottomWidth can you see it

Would we also want to use this variation of the button as the dropdown (and components like it) toggle with left/right padding changes?

@mceledonia do you have an opinion here?
@mcoker I don't see why we couldn't, but it would likely be a breaking change and take a lot of consideration it might get tricky, because when we do it for dropdown, should we also do it for select? and then some selects are built with a div.

@christiemolloy Yeah we would want this styling for all interactive elements in an input group, so both buttons and dropdowns!

Yeah we would want this styling for all interactive elements in an input group

@mceledonia what about for use outside of an input group, could this variation of the button be used as the toggle for standalone dropdown, select, options menu, etc components? The style is

@matthewcarleton has also mentioned there is interest from products in a primary button style toggle for dropdown toggles, too. If we support that, I think it makes sense that the toggle for those components is a variation of our button, whether it's the style we're adding in this PR, the primary button, etc.

just to add to what @mcoker is saying, this issue is tracking what's been seen as a useful addition on the product side. #1872

@mcoker @mattnolting should I make this not a breaking change by putting back in the code for copy to clipboard etc. or do you want to leave this until the breaking change period?

@christiemolloy I don't see the problem with calling this 'input'. An input group is always used to supply an input, right? That may be from a text field as in the search or a select list.

Regarding the disabled question, keep in mind that if this will be disabled then the entire input group will show disabled, i.e. it does not make any sense to allow the user to type into the search field and then have the action/input disabled. Don't know if this changes any thinking on this, but wanted to make sure that we are thinking of the entire input group as one thing.

@mcarrano I'm not sure if we could say that pf-m-input would hold when its used in a dropdown component?

@christiemolloy I don't see the problem with calling this 'input'. An input group is always used to supply an input, right? That may be from a text field as in the search or a select list.

@mcarrano Also can be used with textareas, html selects, dropdown, custom select, other things down the road. That's why we thought pf-m-control worked, because it implies that it controls something, but not necessarily always an input :)

@christiemolloy @mattnolting I must be missing a use case here. In what circumstances would this be used as part of a dropdown, a text area, or another control? I though this was specific to an input group of things built from an input group like a Touchspin.

Sorry about not communicating fully. The potential plan down the road would be to replace the button in the dropdown, select, context selector etc components with this button, as they all share exactly the same styles. This will allow us to reuse a lot of code and make it easier to update all components that use this button @mcarrano

@christiemolloy Thanks for the clarification. I understand better now. I'd say go with pf-m-control unless someone has a better idea.

this comes with .pf-c-button

same ^

any reason you're using ::before instead of ::after? Seems like you would get most of the definitions for free, then you can just redefine --pf-c-button--BorderWidth and handle the border-color somehow. Either the way you have it where you define a new border-color property for the control modifier, or you could separate the default border: var(--pf-c-button--BorderWidth) solid var(--pf-c-button--BorderColor); into border: var(--pf-c-button--BorderWidth) solid; and border-color: var(--pf-c-button--BorderColor);, then define:

--pf-c-button--m-control--before--BorderColor: var(--pf-c-button--m-control--before--BorderTopColor) var(--pf-c-button--m-control--before--BorderRightColor) var(--pf-c-button--m-control--before--BorderBottomColor) var(--pf-c-button--m-control--before--BorderLeftColor);

then redefine --pf-c-button--BorderColor: var(--pf-c-button--m-control--before--BorderColor); in &.pf-m-control {}

Ah the reason I used before instead of after was because I based it off the clipboard-copy-group-copy, but after definitely makes sense in this case. Updated.

This works for me, though we could also be safe and create --pf-c-button--m-control--disabled--before--BorderBottomColor, which allows flexbility for this to be its own var if we want to change it in the future without a breaking change. Or if you go with the approach above and create --pf-c-button--m-control--before--BorderColor, then you could create --pf-c-button--m-control--disabled--before--BorderColor and just redefine --pf-c-button--m-control--before--BorderColor: var(--pf-c-button--m-control--disabled--before--BorderColor) here and you wouldn't have to write out the pseudo class selector.

Suggested change

      
            | `.pf-m-control` | `.pf-c-button` | Modifies for control styles. **Note: ** This modifier should only be used when using buttons in the Input or ClipboardCopy components. |
          
            | `.pf-m-control` | `.pf-c-button` | Modifies for control styles. **Note:** This modifier should only be used when using buttons in the Input Group or Clipboard Copy components. |

Maybe instead of "to be expanded" change to "for the expanded state" or something. Currently it reads like the button is expandable, instead of representing the expanded state of something else.

Do you think this variations and "link" and "inline link" above need the word "button" in the examples?

No I think its redundant, updated all the examples to remove button

the context selector and input group examples should use pf-m-control instead of pf-m-input

Looks great! Left some feedback, let me know if you have any questions.

Any reason we wouldn't just use 0 here? We use unset very sparingly, and in this context, while it will still evaluate to initial (which is 0 for this property) we could just set that directly since we don't want it to have a border-radius.

Suggested change

      
            {{#> button button--modifier="pf-m-control" button--attribute='aria-label="copy input"'}}
          
            {{#> button button--modifier="pf-m-control" button--attribute='aria-label="Copy input"'}}

super nit - would you mind sentence casing these aria-label values?

Suggested change

      
              {{#> form-control controlType="input" input="true" form-control--attribute='type="text" id="textInput10" name="textInput10" aria-label="input example with popover"'}}
          
              {{#> form-control controlType="input" input="true" form-control--attribute='type="text" id="textInput10" name="textInput10" aria-label="Input example with popover"'}}

Updated the file, most of the aria-labels weren't capitalized

CSS looks great! Left a couple of small comments. Can you also modify the input group so that it doesn't apply the border-radius to .pf-m-control buttons?

Nice job @christiemolloy!!

@christiemolloy @mcarrano is the goal to not support the previous ways of attaching buttons to the input group component where we square the borders except for the left side of the first button and right side of the last one? Or are we going to continue to allow that indefinitely?

If we aren't going to allow it, we should create a follow up issue to remove the CSS that does that in a breaking change release.

🎉 This PR is included in version 2.32.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

# 2019.08 release notes (2019-10-01) Packages released: - @patternfly/patternfly@2.x ## Components - **Charts:** - Adjust padding for pie and donut charts ([#2247](#2247)) - Set mix-blend-mode for better color contrast ([#2239](#2239)) - Update area chart opacity ([#2233](#2233)) - **Copy to clipboard:** Wrapped code in pre which allows the code to preserve line-breaks and spaces and also applies styling such as the monospace font. ([#2260](#2260)) - **Data list:** Make data list borders consistent with table ([#2289](#2289)) - **Data toolbar:** - Updated clear button mods ([#2248](#2248)) - Added in examples for expandable ([#2273](#2273)) - **Flex layout:** Wrapped flex items, update css ([#2214](#2214)) - **Wizard:** - Re-enabled wizard modal demos ([#2259](#2259)) - Updated in page wizard demos to use dark sidebar/nav ([#2296](#2296)) - **Navigation:** Added styling updates to dark nav ([#2283](#2283)) - **Page:** Added ability to keep sidebar collapsed between sizes ([#2264](#2264)) - **Pagination:** Added compact variation ([#2275](#2275)) - **Radio:** Updated the radio component example to single components ([#2231](#2231)) - **Table:** - Fixed a11y issues in org documentation/html/table ([#2282](#2282)) - Added empty and loading state table demos ([#2254](#2254)) - **Tooltip:** Added text align left modifier ([#2284](#2284)) - **Select:** - Added typeahead form wrapper, udpated css ([#2255](#2255)) - Add top expanded example ([#2026](#2026)) - **Chip group:** Add overflow to group ([#2278](#2278)) - **Button:** Add control modifier ([#2005](#2005)) ## Other - **Shield:** Added missing components, missing var to brand docs js ([#2263](#2263))

* Create 2019.08 release notes (2019-10-01) Adding new release notes * Update 2019.08 release notes (2019-10-01) # 2019.08 release notes (2019-10-01) Packages released: - @patternfly/patternfly@2.x ## Components - **Charts:** - Adjust padding for pie and donut charts ([#2247](#2247)) - Set mix-blend-mode for better color contrast ([#2239](#2239)) - Update area chart opacity ([#2233](#2233)) - **Copy to clipboard:** Wrapped code in pre which allows the code to preserve line-breaks and spaces and also applies styling such as the monospace font. ([#2260](#2260)) - **Data list:** Make data list borders consistent with table ([#2289](#2289)) - **Data toolbar:** - Updated clear button mods ([#2248](#2248)) - Added in examples for expandable ([#2273](#2273)) - **Flex layout:** Wrapped flex items, update css ([#2214](#2214)) - **Wizard:** - Re-enabled wizard modal demos ([#2259](#2259)) - Updated in page wizard demos to use dark sidebar/nav ([#2296](#2296)) - **Navigation:** Added styling updates to dark nav ([#2283](#2283)) - **Page:** Added ability to keep sidebar collapsed between sizes ([#2264](#2264)) - **Pagination:** Added compact variation ([#2275](#2275)) - **Radio:** Updated the radio component example to single components ([#2231](#2231)) - **Table:** - Fixed a11y issues in org documentation/html/table ([#2282](#2282)) - Added empty and loading state table demos ([#2254](#2254)) - **Tooltip:** Added text align left modifier ([#2284](#2284)) - **Select:** - Added typeahead form wrapper, udpated css ([#2255](#2255)) - Add top expanded example ([#2026](#2026)) - **Chip group:** Add overflow to group ([#2278](#2278)) - **Button:** Add control modifier ([#2005](#2005)) ## Other - **Shield:** Added missing components, missing var to brand docs js ([#2263](#2263)) * Rename 2019.08 release notes (2019-10-01) to 2019.08 release notes (2019-10-01).MD Added .md file name * Update 2019.08 release notes (2019-10-01).MD Co-Authored-By: Michael Coker <35148959+mcoker@users.noreply.github.com> * alphabetize components

feat(button): add input modifier

564e37a

fix(button): update

aaf8482

christiemolloy assigned mattnolting, mceledonia and mcoker and unassigned mattnolting, mceledonia and mcoker Jul 1, 2019

fix(button): add aria-label

c9da61d

christiemolloy commented Jul 1, 2019

View reviewed changes

fix(button): update

5fbde85

christiemolloy requested review from mattnolting, mceledonia and mcoker July 1, 2019 17:17

mcoker reviewed Jul 1, 2019

View reviewed changes

mattnolting reviewed Jul 2, 2019

View reviewed changes

mcoker reviewed Jul 2, 2019

View reviewed changes

fix(button): add expanded modifier to button and remove from clipboard

ad6c56c

christiemolloy added the update symbol library label Jul 23, 2019

christiemolloy added 3 commits September 12, 2019 09:32

Merge branch 'master' into iss1926

d3d3b99

Merge branch 'master' into iss1926

68dbd17

feat(button): make non-breaking change

8e81c4f

mcoker reviewed Sep 17, 2019

View reviewed changes

fix(button): based on cokers review

ffbf3fd

mcoker reviewed Sep 18, 2019

View reviewed changes

mcoker requested changes Sep 18, 2019

View reviewed changes

fix(button): remove border radius on control buttons

096539d

mcoker approved these changes Sep 18, 2019

View reviewed changes

mcoker merged commit 1d93536 into patternfly:master Sep 19, 2019

redallen added the released label Sep 19, 2019

christiemolloy mentioned this pull request Sep 19, 2019

feat(button): add "control" prop for styling patternfly/patternfly-react#2963

Closed

maryshak1996 mentioned this pull request Sep 19, 2019

Touchspin #362

Closed

+                // --pf-c-clipboard-copy__group-toggle--active--BorderBottomColor: var(--pf-global--active-color--100);
+                // --pf-c-clipboard-copy__group-toggle--focus--BorderBottomWidth: var(--pf-global--BorderWidth--md);
+                // --pf-c-clipboard-copy__group-toggle--focus--BorderBottomColor: var(--pf-global--active-color--100);
+                --pf-c-clipboard-copy__group-toggle--c-button--m-expanded--BorderBottomWidth: var(--pf-global--BorderWidth--md);

@@ @@ -3,11 +3,15 @@ Editable @@
               {{#> clipboard-copy clipboard-copy--id="3"}}
               	{{#> clipboard-copy-group}}
                   {{#> clipboard-copy-group-toggle clipboard-copy-group-toggle--attribute=(concat 'id="toggle-' clipboard-copy--id '" aria-labelledby="toggle-' clipboard-copy--id ' text-input-' clipboard-copy--id '" aria-controls="content-' clipboard-copy--id '"')}}

               | `.pf-m-plain` | `.pf-c-button` | Modifies for icon styles. This button has no background or border, uses a standard text color, and is used for `.pf-m-plain` icon buttons such as close, expand, kebab, etc. |
               | `.pf-m-inline` | `.pf-c-button.pf-m-link` | Modifies for inline styles. This button is presented similar to a normal link, has no padding, and is displayed inline with other inline content. |
               | `.pf-m-block` | `.pf-c-button` | Creates a block level button. |
+              | `.pf-m-control` | `.pf-c-button` | Modifies for control styles. **Note: ** This modifier should only be used when using buttons in the Input or ClipboardCopy components. |

               | `.pf-m-inline` | `.pf-c-button.pf-m-link` | Modifies for inline styles. This button is presented similar to a normal link, has no padding, and is displayed inline with other inline content. |
               | `.pf-m-block` | `.pf-c-button` | Creates a block level button. |
+              | `.pf-m-control` | `.pf-c-button` | Modifies for control styles. **Note: ** This modifier should only be used when using buttons in the Input or ClipboardCopy components. |
+              | `.pf-m-expanded` | `.pf-c-button.pf-m-input` | Modifies an input button to be expanded. |

               {{/button}}
+              <br><br>
+              {{#> button button--modifier="pf-m-control"}}
+                Control button

		@@ -1,4 +1,4 @@
		<button class="pf-c-clipboard-copy__group-toggle{{#if clipboard-copy-group-toggle--modifier}} {{clipboard-copy-group-toggle--modifier}}{{/if}}"
		<div class="pf-c-clipboard-copy__group-toggle{{#if clipboard-copy-group-toggle--modifier}} {{clipboard-copy-group-toggle--modifier}}{{/if}}"

                 }
+                &.pf-m-control {
+                  position: relative;

+                  position: relative;
+                  border: none;
+                  &::before {

+                    background-color: var(--pf-c-button--m-control--disabled--BackgroundColor);
+                    &::before {
+                      --pf-c-button--m-control--before--BorderBottomColor: var(--pf-c-button--m-control--before--BorderTopColor);

	\| `.pf-m-control` \| `.pf-c-button` \| Modifies for control styles. Note: This modifier should only be used when using buttons in the Input or ClipboardCopy components. \|
	\| `.pf-m-control` \| `.pf-c-button` \| Modifies for control styles. Note: This modifier should only be used when using buttons in the Input Group or Clipboard Copy components. \|

                       {{#> form-control controlType="input" input="true" form-control--attribute=(concat 'type="search"' 'placeholder="Search"' 'id="textInput1"' 'name="textInput1"' 'aria-labelledby="' context-selector--id '-search-button"')}}
                       {{/form-control}}
-                      {{#> button button--modifier="pf-m-tertiary" button--attribute=(concat 'id="' context-selector--id '-search-button"' 'aria-label="Search menu items"')}}
+                      {{#> button button--modifier="pf-m-input" button--attribute=(concat 'id="' context-selector--id '-search-button"' 'aria-label="Search menu items"')}}

+                    --pf-c-button--BorderWidth: var(--pf-c-button--m-control--after--BorderWidth);
+                    --pf-c-button--BorderColor: var(--pf-c-button--m-control--after--BorderTopColor) var(--pf-c-button--m-control--after--BorderRightColor) var(--pf-c-button--m-control--after--BorderBottomColor) var(--pf-c-button--m-control--after--BorderLeftColor);
+                    border-radius: unset;

	{{#> button button--modifier="pf-m-control" button--attribute='aria-label="copy input"'}}
	{{#> button button--modifier="pf-m-control" button--attribute='aria-label="Copy input"'}}

		@@ -96,7 +96,7 @@
		{{#> input-group}}
		{{#> form-control controlType="input" input="true" form-control--attribute='type="text" id="textInput10" name="textInput10" aria-label="input example with popover"'}}

feat(button): add input modifier by christiemolloy · Pull Request #2005 · patternfly/patternfly · GitHub

Uh oh!

feat(button): add input modifier - #2005

feat(button): add input modifier#2005mcoker merged 15 commits intopatternfly:masterpatternfly/patternfly:masterfrom christiemolloy:iss1926christiemolloy/patternfly-next:iss1926Copy head branch name to clipboard

Conversation

christiemolloy commented Jul 1, 2019

Uh oh!

patternfly-build commented Jul 1, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mcoker commented Jul 1, 2019

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!

mattnolting 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!

christiemolloy commented Jul 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

christiemolloy commented Jul 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mceledonia commented Jul 10, 2019

Uh oh!

mcoker commented Jul 10, 2019

Uh oh!

matthewcarleton commented Jul 12, 2019

Uh oh!

christiemolloy commented Jul 17, 2019

Uh oh!

mcarrano commented Sep 17, 2019

Uh oh!

christiemolloy commented Sep 17, 2019

Uh oh!

mattnolting commented Sep 17, 2019

Uh oh!

mcarrano commented Sep 17, 2019

Uh oh!

christiemolloy commented Sep 17, 2019

Uh oh!

mcarrano commented Sep 17, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mcoker Sep 17, 2019 • 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!

mcoker Sep 17, 2019 • 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

feat(button): add input modifier#2005
mcoker merged 15 commits into
patternfly:masterfrom
christiemolloy:iss1926

patternfly-build commented Jul 1, 2019 •
edited
Loading

christiemolloy commented Jul 2, 2019 •
edited
Loading

christiemolloy commented Jul 2, 2019 •
edited
Loading

mcoker Sep 17, 2019 •
edited
Loading

mcoker Sep 17, 2019 •
edited
Loading