feat(TextInputGroup): added validation by thatblindgeye · Pull Request #6802 · patternfly/patternfly

thatblindgeye · 2024-06-17T15:58:36Z

Closes #6654 , closes #6796

patternfly-build · 2024-06-17T16:01:20Z

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

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

andrew-ronaldson

This looks good to me. One unrelated nit is that we talked about moving the X button to flex-start instead of center. Could we squeeze that in or make its own issue

thatblindgeye · 2024-06-26T12:47:14Z

@mcoker do you think this would suffice for the above?

// starting at line 215
.#{$text-input-group}__utilities {
  ...existing styles
  > button {
    align-self: flex-start;
  }
}

(or the inverse, update align-items to flex-start then in a badge selector make align-self center)

We can't apply the flex-start to the utilities wrapper since that may contain badges/other things which should remain centered. Unless we want to add some modifier class on the outermost text-input-group class that will determine the align-items value on .#{$text-input-group}__utilities (in possibly the edge case where a filter expanded text input group may contain a "clear" button and a result count?)

If there's more to consider then I can make a followup issue, but could possibly be a quick enough addition for this PR

mcoker · 2024-06-26T17:19:11Z

@thatblindgeye if you rebase this PR, the a11y errors should go away since we removed the context selector component.

mcoker · 2024-06-26T22:29:35Z

@thatblindgeye @andrew-ronaldson do we want badges and other actions in the utilities element to be centered vertically all the time for both single-line and multi-line text input group layouts? Or should everything be vertically centered in a single-line text input group, but be start aligned (vertically centered with the first line of content) in a mult-line text input group?

andrew-ronaldson · 2024-06-27T15:31:52Z

If the children of the utilities container were aligned to top would we need extra margin around the badge to make it visually centered with the X button.

mattnolting

Great work @thatblindgeye! Only thing I'm noticing is icon positioning. Following comments will address.

Before:

Update:

src/patternfly/components/TextInputGroup/text-input-group-template-status.hbs

mattnolting · 2024-07-02T19:26:30Z

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

+
  &.pf-m-icon {
    --#{$text-input-group}__text-input--PaddingInlineStart: var(--#{$text-input-group}__main--m-icon__text-input--PaddingInlineStart);
  }


Suggested change

}

display: inline-flex;

align-items: center;

justify-content: center;

min-width: calc(var(--#{$text-input-group}__LineHeight) * --#{$text-input-group}__FontSize));

}

Add these to :where(:root)

--#{$text-input-group}__LineHeight): var(--pf-t--global--font--line-height--body); --#{$text-input-group}__FontSize: var(--pf-t--global--font--size--body--default);

Add these to .#{$text-input-group}

line-height: var(--#{$text-input-group}__LineHeight); font-size: var(--#{$text-input-group}__FontSize);

mcoker

Looks fantastic. I left some comments and am also curious about a couple of things:

We're drawing the border on the outer text input group wrapper from the child __main element. That works now, but if we need to add position: relative to __main, we'll no longer be able to create the border from __main. Can we move both the status modifiers and the visible border to the outer .pf-v6-c-text-input-group element?
What's the role of .pf-m-status? One thing is I see we're adding selectors like .thing:where(.pf-m-success, .pf-m-error, ...)... we could require .pf-m-status to be used with any kind of status on an element like class="pf-m-status pf-m-error", and then instead of listing all of the classes in our selector, we could just target .pf-m-status.

src/patternfly/components/TextInputGroup/text-input-group-template-status.hbs

mcoker · 2024-07-02T18:32:37Z

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

  --#{$text-input-group}__text--BorderStartEndRadius: var(--#{$text-input-group}__text--BorderRadius--base);
  --#{$text-input-group}__text--BorderEndEndRadius: var(--#{$text-input-group}__text--BorderRadius--base);
  --#{$text-input-group}__text--BorderEndStartRadius: var(--#{$text-input-group}__text--BorderRadius--base);
+  --#{$text-input-group}__text--Position: initial;


This probably isn't doing what you intend? Using initial as the value for a css custom property will force the fallback to be triggered in any var() function where it's used.

For example, this will make .thing have a background color of "red"

--background: initial; .thing { background: var(--background, red); }

If you wanted "initial" as the default value for the position prop where this is used, you'd just leave --#{$text-input-group}__text--Position undefined by default, and use "initial" as the fallback directly in the property declaration:

.#{$text-input-group}__text { position: var(--#{$text-input-group}__text--Position, initial);

Though I'd recommend using "revert" instead of "initial" - it's fine here, but "initial" doesn't always return what you're looking for. For example, if you had like div { display: initial; } it would change div to have display: inline since "inline" is the initial value for display

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

andrew-ronaldson · 2024-07-09T17:41:16Z

Changes look good Eric! I will close the related issue for the border-radius

mcoker

🐳

patternfly-build · 2024-07-09T20:38:07Z

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

The release is available on:

Your semantic-release bot 📦🚀

thatblindgeye requested review from andrew-ronaldson and mcoker June 17, 2024 15:58

thatblindgeye linked an issue Jun 17, 2024 that may be closed by this pull request

Text input group - support for validation #6654

Closed

andrew-ronaldson approved these changes Jun 26, 2024

View reviewed changes

thatblindgeye force-pushed the iss6654_tigValidation branch from 9c50832 to 0f01d3e Compare June 26, 2024 17:31

mattnolting requested changes Jul 2, 2024

View reviewed changes

mcoker requested changes Jul 2, 2024

View reviewed changes

thatblindgeye force-pushed the iss6654_tigValidation branch from 0f01d3e to b64c26b Compare July 3, 2024 12:30

thatblindgeye requested review from mattnolting and mcoker July 3, 2024 12:30

Eric Olkowski added 3 commits July 9, 2024 09:50

feat(TextInputGroup): added validation

45521c9

Fixed icon spacing, moved border to outer wrapper

34eca92

Fixed disabled border radius

a33ec31

thatblindgeye force-pushed the iss6654_tigValidation branch from b64c26b to a33ec31 Compare July 9, 2024 13:50

thatblindgeye linked an issue Jul 9, 2024 that may be closed by this pull request

Text input group - disabled lost border-radius #6796

Closed

andrew-ronaldson mentioned this pull request Jul 9, 2024

Text input group - disabled lost border-radius #6796

Closed

Eric Olkowski added 2 commits July 9, 2024 15:52

Async feedback

f4f7b3a

Updated hover placement

212216a

mcoker approved these changes Jul 9, 2024

View reviewed changes

mcoker merged commit 57a319e into patternfly:v6 Jul 9, 2024

This was referenced Jul 9, 2024

TextInputGroup - alignment for utilities is off #6867

Closed

TextInputGroup - support validation patternfly/patternfly-react#10720

Closed

patternfly-build added the released on @alpha label Jul 9, 2024

patternfly-build mentioned this pull request Apr 16, 2025

The automated release is failing 🚨 #7464

Closed

-  }
+    display: inline-flex;
+    align-items: center;
+    justify-content: center;
+    min-width: calc(var(--#{$text-input-group}__LineHeight) * --#{$text-input-group}__FontSize));
+  }

Conversation

thatblindgeye commented Jun 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

andrew-ronaldson left a comment

Choose a reason for hiding this comment

Uh oh!

thatblindgeye commented Jun 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mcoker commented Jun 26, 2024

Uh oh!

mcoker commented Jun 26, 2024

Uh oh!

andrew-ronaldson commented Jun 27, 2024

Uh oh!

mattnolting left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mattnolting Jul 2, 2024

Choose a reason for hiding this comment

Uh oh!

mattnolting Jul 2, 2024

Choose a reason for hiding this comment

Uh oh!

mattnolting Jul 2, 2024

Choose a reason for hiding this comment

Uh oh!

mcoker left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mcoker Jul 2, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

andrew-ronaldson commented Jul 9, 2024

Uh oh!

mcoker left a comment

Choose a reason for hiding this comment

Uh oh!

patternfly-build commented Jul 9, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

thatblindgeye commented Jun 17, 2024 •

edited

Loading

patternfly-build commented Jun 17, 2024 •

edited

Loading

thatblindgeye commented Jun 26, 2024 •

edited

Loading