Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Validated inputs in .input-group shouldn't be behind sibling element #33644

Merged
merged 3 commits into from May 13, 2021

Conversation

jatinmehta
Copy link
Contributor

@jatinmehta jatinmehta commented Apr 15, 2021

Reverted changes done in PR #33211 to fix #33541

@jatinmehta jatinmehta requested a review from a team as a code owner April 15, 2021 12:55
@jatinmehta jatinmehta changed the title Removed change done by #33211 Removed changes done by #33211 Apr 15, 2021
@jatinmehta jatinmehta changed the title Removed changes done by #33211 Removed changes done by #33211 to solve issue #33541 Apr 15, 2021
@ffoodd
Copy link
Member

ffoodd commented Apr 15, 2021

What about scoping the z-index to the invalid state instead?

Reverting the fix will obviously reintroduce the initial issue, and I think we can solve both cases by only checking that $state is invalid.

@jatinmehta jatinmehta force-pushed the border-hidden-input-validation branch from f4fc1e7 to 0ce1c16 Compare April 15, 2021 18:10
@jatinmehta
Copy link
Contributor Author

@ffoodd I have made the required changes, does this solve both the problems?

@korki43
Copy link

korki43 commented Apr 16, 2021

Scoping the z-index to the invalid state would help with a valid input next to an invalid one, but if two invalid inputs are directly next to each other this won't work.

@jatinmehta the scss you introduced in the last commit won't work. You have to create the selectors using the .is-invalid class and .was-validated with the css pseudo-selector :invalid.

Perhaps we could scope the z-index to a focused input element. I'll have a look on it.

@korki43
Copy link

korki43 commented Apr 16, 2021

The following patch (on top of main) will solve most issues (demo).
The middle border will stay in the color of the right element except when the left element is focused, which seems ok to me.

diff --git a/scss/mixins/_forms.scss b/scss/mixins/_forms.scss
index 283462fd5..e86bd8aef 100644
--- a/scss/mixins/_forms.scss
+++ b/scss/mixins/_forms.scss
@@ -130,7 +130,9 @@
   .input-group .form-control,
   .input-group .form-select {
     @include form-validation-state-selector($state) {
-      z-index: 3;
+      &:focus {
+        z-index: 3;
+      }
     }
   }
 }

@ffoodd
Copy link
Member

ffoodd commented Apr 16, 2021

@korki43 Might seems OK to you but you're reintroducing #33204. The previous fix intended to ensure validation state are above; #33541 is about making invalid state above valid state.

I'd be in favor of a local scale, with valid state being 2 and invalid state being 3. This way we ensure validation states are above default state, and that invalid state is above valid state.

@jatinmehta You need to keep the form-validation-state-slector mixin and check the state inside. I'll suggest a thing.

scss/mixins/_forms.scss Outdated Show resolved Hide resolved
@ffoodd
Copy link
Member

ffoodd commented Apr 16, 2021

I also forked @korki43's pen to use this PR's dist file directly. Thanks for the test case!

@jatinmehta jatinmehta force-pushed the border-hidden-input-validation branch from a0f8c4f to 7f641a9 Compare April 16, 2021 18:07
@ffoodd
Copy link
Member

ffoodd commented Apr 16, 2021

Based on the Pen using last commit, I think we should add a rule regarding :focus too, so that focused input is above any other one.

Something like:

&:focus {
 z-index: 3;
}

Right after the @else if block.

Andy thought?

@jatinmehta
Copy link
Contributor Author

@ffoodd I think adding z-index: 3 for focus as well will solve all the issues.

@mdo mdo requested a review from ffoodd April 17, 2021 21:39
@jatinmehta jatinmehta force-pushed the border-hidden-input-validation branch from 7f641a9 to 074eebc Compare April 18, 2021 15:09
Jatin Mehta and others added 2 commits April 18, 2021 20:49
Co-authored-by: Gaël Poupard <ffoodd@users.noreply.github.com>
@jatinmehta jatinmehta force-pushed the border-hidden-input-validation branch from 074eebc to 988f7f5 Compare April 18, 2021 15:19
@jatinmehta
Copy link
Contributor Author

@ffoodd I have made the requested changes. Can this PR be merged now?

Copy link
Member

@ffoodd ffoodd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, I updated my CodePen to ease testing various combinations and it works great.

Thanks for your patience and involvement!

@XhmikosR
Copy link
Member

@mdo should we include this in v5.0.0?

@jatinmehta
Copy link
Contributor Author

@mdo Can this PR be merged now?

@korki43
Copy link

korki43 commented Apr 20, 2021

@jatinmehta no worries, it'll be merged. Just wait for the formal questions to be discussed.

@mdo mdo changed the title Removed changes done by #33211 to solve issue #33541 Validated inputs in .input-group shouldn't be behind sibling element May 13, 2021
@mdo mdo added this to Inbox in v5.0.1 via automation May 13, 2021
v5.0.1 automation moved this from Inbox to Approved May 13, 2021
@XhmikosR XhmikosR merged commit d81d0a9 into twbs:main May 13, 2021
v5.0.1 automation moved this from Approved to Done May 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
No open projects
v5.0.1
  
Done
Development

Successfully merging this pull request may close these issues.

Border indicating failed input field validation in input group is hidden behind its neighbor element
6 participants