fix(TextInputGroup): updated inline padding of icons by thatblindgeye · Pull Request #6945 · patternfly/patternfly

thatblindgeye · 2024-08-05T14:13:51Z

Closes #6930

Couple of notes;

The gap between the status icon and utilities is slightly larger now.
This PR:

Staging:
Did we want padding at the end of the utilities, to create more of a visual gap? Right now when the close button is hovered there's not much of a gap:

But if we used the same spacing as the icons it may create too much of a visual gap when the button isn't focused:

patternfly-build · 2024-08-05T14:16:34Z

Preview: https://patternfly-pr-6945.surge.sh

A11y report: https://patternfly-pr-6945-a11y.surge.sh

mcoker · 2024-08-05T15:54:15Z

Here's a screenshot of some text input groups with the utilities/close icon inset by the control/horizontal spacer

And here is the same, but with the utilities/close offset by the close button's horizontal padding, pushing it back to the right by the close button's right padding, which will make the close button's icon ("x") visually inset to the control/horizontal spacer, but on hover, you can see the actual button's right edge is closer to the right edge of the text input group

mcoker · 2024-08-05T16:18:51Z

From 8.5.24 working session w/ @lboehling

Close button is fine on the right edge, @lboehling likes the hover/focus background to be flush with the right edge of the TIG. Assuming plain actions will always be in utilities (as our recommendation anyways), that should work fine

The text-input right padding when there is a status icon is set to the 'xl' spacer. We should update that to match the left padding when there is an icon, if possible, which would be something like --pf-t--[spacer-control-horizontal + icon-size + text-to-element like this

patternfly/src/patternfly/components/TextInputGroup/text-input-group.scss

Line 22 in 031ffb6

    
           --#{$text-input-group}__main--m-icon__text-input--PaddingInlineStart: calc(var(--pf-t--global--spacer--control--horizontal--default) + var(--pf-v6-c-text-input-group__icon--FontSize) + var(--pf-t--global--spacer--gap--text-to-element--default));

If the TIG has utilities, we should reduce the right padding of the text-input to something reasonable to separate text/icons/whatever in the text input from the utilities to the right of it. Probably a small spacer would work well?

mcoker

Everything looks great! Just a couple of nits on the naming and token used in one spot. Lemme know what you think!

mcoker · 2024-08-07T16:04:26Z

src/patternfly/components/TextInputGroup/text-input-group.scss

-  --#{$text-input-group}__icon--m-status--InsetInlineEnd: var(--pf-t--global--spacer--sm);
+  --#{$text-input-group}__icon--InsetInlineStart: var(--pf-t--global--spacer--control--horizontal--default);
+  --#{$text-input-group}__icon--m-status--InsetInlineEnd: var(--pf-t--global--spacer--control--horizontal--default);
+  --#{$text-input-group}__c-utilities__icon--m-status--InsetInlineEnd: var(--pf-t--global--spacer--control--horizontal--compact);


Just a couple of nits on this

The syntax for this var is flexible since it's a pretty uncommon pattern. Though typically we reserve the c-[whatever] naming when we reference another component (eg c-button). We'll use --[component/BEM element] (eg --c-button or --utilities) as a general identifier (not necessarily a child or direct child, just in there somewhere) so I wonder if the suggestion below is better?

Also not sure if --pf-t--global--spacer--control--horizontal--compact) is the right spacer. Reason being, if a person themed their compact control padding, I wouldn't expect this spacing to update? Since it doesn't necessarily need to conform to an outer button/control padding to match other controls, I think we could just use the --spacer--sm token here. WDYT?

Suggested change

--#{$text-input-group}__c-utilities__icon--m-status--InsetInlineEnd: var(--pf-t--global--spacer--control--horizontal--compact);

--#{$text-input-group}--utilities--icon--m-status--InsetInlineEnd: var(--pf-t--global--spacer--control--horizontal--compact);

andrew-ronaldson · 2024-08-12T13:22:02Z

Noticed this example has a broken status icon. When I inspect the warning example above it has an `inset-inline-end: var(--pf-v6-c-text-input-group__icon--m-status--InsetInlineEnd)` but it says that same var is undefined on the error example

thatblindgeye · 2024-08-12T13:33:51Z

@andrew-ronaldson should be fixed now, forgot to update the other instance of the CSS var that was renamed in the latest update

andrew-ronaldson

sweet moves, ace!

mcoker

Looks awesome, just one nit comment that I think should be pretty straight forward?

mcoker · 2024-08-13T02:37:27Z

src/patternfly/components/TextInputGroup/text-input-group.scss

  --#{$text-input-group}__main--first-child--not--text-input--MarginInlineStart: calc(var(--pf-t--global--spacer--control--horizontal--plain) / 2);
  --#{$text-input-group}__main--m-icon__text-input--PaddingInlineStart: calc(var(--pf-t--global--spacer--control--horizontal--default) + var(--pf-v6-c-text-input-group__icon--FontSize) + var(--pf-t--global--spacer--gap--text-to-element--default));
-  --#{$text-input-group}--status__text-input--PaddingInlineEnd: var(--pf-t--global--spacer--xl);
+  --#{$text-input-group}--status__text-input--PaddingInlineEnd: calc(var(--pf-t--global--spacer--control--horizontal--default) + var(--pf-v6-c-text-input-group__icon--FontSize) + var(--pf-t--global--spacer--gap--text-to-element--default));


This is awesome. But when the close button is present, we adjust the status button's inset from --pf-t--global--spacer--control--horizontal--default to --pf-t--global--spacer--sm with the other change in this PR. That leaves a little too much padding between the status icon and text in the text input.

WDYT about creating a variation of this var that swaps --pf-t--global--spacer--control--horizontal--default with --pf-t--global--spacer--sm? Then it looks like this, with the visual spacer being the text-to-element spacer:

Pushed, lemme know if that's what you had in mind. FWIW updating the existing var so that it starts as calc(var(--#{$text-input-group}__icon--m-status--InsetInlineEnd)... also worked, though I imagine if a consumer is updating that specific var we wouldn't want that to affect the spacing between status icon and text input.

mcoker

NICE!

patternfly-build · 2024-08-19T23:51:00Z

🎉 This PR is included in version 6.0.0-alpha.217 🎉

The release is available on:

Your semantic-release bot 📦🚀

thatblindgeye requested review from andrew-ronaldson and mcoker August 5, 2024 14:13

thatblindgeye linked an issue Aug 5, 2024 that may be closed by this pull request

Text input group inset bugs with icons and validation #6930

Closed

thatblindgeye requested a review from lboehling August 6, 2024 12:23

mcoker reviewed Aug 7, 2024

View reviewed changes

thatblindgeye force-pushed the iss6930_tigInsetIcons branch from 7cf0c16 to d4d0bd9 Compare August 12, 2024 13:05

thatblindgeye requested a review from mcoker August 12, 2024 13:06

thatblindgeye force-pushed the iss6930_tigInsetIcons branch from d4d0bd9 to 59ce58b Compare August 12, 2024 13:33

andrew-ronaldson approved these changes Aug 12, 2024

View reviewed changes

mcoker reviewed Aug 13, 2024

View reviewed changes

Eric Olkowski added 4 commits August 14, 2024 08:32

fix(TextInputGroup): updated inline padding of icons

6ad66ff

Updated spacing around status icon

1d17f82

Updated CSS var name/value

149eb48

Added var for status icon padding with utilities

a17862c

thatblindgeye force-pushed the iss6930_tigInsetIcons branch from 59ce58b to a17862c Compare August 14, 2024 12:44

mcoker approved these changes Aug 19, 2024

View reviewed changes

mcoker merged commit 67ebfac into patternfly:v6 Aug 19, 2024

mcoker mentioned this pull request Aug 19, 2024

Text input group inset bugs with icons and validation #6930

Closed

patternfly-build added the released on @alpha label Aug 19, 2024

mcoker mentioned this pull request Aug 20, 2024

fix(toolbar-group): let label groups wrap within the toolbar #6987

Merged

patternfly-build mentioned this pull request Apr 16, 2025

The automated release is failing 🚨 #7464

Closed

	--#{$text-input-group}__c-utilities__icon--m-status--InsetInlineEnd: var(--pf-t--global--spacer--control--horizontal--compact);
	--#{$text-input-group}--utilities--icon--m-status--InsetInlineEnd: var(--pf-t--global--spacer--control--horizontal--compact);

Conversation

thatblindgeye commented Aug 5, 2024

Uh oh!

patternfly-build commented Aug 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mcoker commented Aug 5, 2024

Uh oh!

mcoker commented Aug 5, 2024

Uh oh!

mcoker left a comment

Choose a reason for hiding this comment

Uh oh!

mcoker Aug 7, 2024

Choose a reason for hiding this comment

Uh oh!

andrew-ronaldson commented Aug 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thatblindgeye commented Aug 12, 2024

Uh oh!

andrew-ronaldson left a comment

Choose a reason for hiding this comment

Uh oh!

mcoker left a comment

Choose a reason for hiding this comment

Uh oh!

mcoker Aug 13, 2024

Choose a reason for hiding this comment

Uh oh!

thatblindgeye Aug 14, 2024

Choose a reason for hiding this comment

Uh oh!

mcoker left a comment

Choose a reason for hiding this comment

Uh oh!

patternfly-build commented Aug 19, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

patternfly-build commented Aug 5, 2024 •

edited

Loading

andrew-ronaldson commented Aug 12, 2024 •

edited

Loading