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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix validation feedback icon in select multiple #33411

Merged
merged 1 commit into from Apr 2, 2021

Conversation

tagliala
Copy link
Contributor

Hi! 馃憢馃徏馃憢馃徏馃憢馃徏

It has been a long time I do not contribute to Bootstrap. I've searched for "select validation feedback" but I didn't find open issues or other PRs, so apologies if it is a duplicate or if the behavior is intended

Validation feedback for <select multiple> should look like
<textarea>.

The previous implementation was placing the validation icon in the
middle of the select field together with the single select arrow, that
is not supposed to be part of this kind of inputs.

How to replicate

  1. Go to https://getbootstrap.com/docs/5.0/forms/select/#sizing
  2. Add is-invalid or is-valid class to one of the <select multiple> or <select size="n>1"> inputs

What did you get

image

What did you expect (this PR)

image

@tagliala tagliala requested a review from a team as a code owner March 19, 2021 21:08
@twbs twbs deleted a comment Mar 23, 2021
@tagliala tagliala force-pushed the fix-select-multiple-validation-feedback branch from b3e244c to 7de3454 Compare March 23, 2021 18:59
Validation feedback for `<select multiple>` should look like
`<textarea>`.

The previous implementation was placing the validation icon in the
middle of the select field together with the single select arrow, that
is not supposed to be part of this kind of inputs
@tagliala tagliala force-pushed the fix-select-multiple-validation-feedback branch from 7de3454 to 1eba8ee Compare March 30, 2021 19:34
Copy link
Member

@mdo mdo left a comment

Choose a reason for hiding this comment

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

LGTM, thoughts @ffoodd?

@mdo mdo added this to Inbox in v5.0.0 via automation Apr 1, 2021
@mdo mdo merged commit bfafabf into twbs:main Apr 2, 2021
v5.0.0 automation moved this from Inbox to Done Apr 2, 2021
@ffoodd
Copy link
Member

ffoodd commented Apr 3, 2021

Late to the party, sorry. I guess we could have wrap the previous declaration in a &:not(...) selector, but I'm not sure it'd be simpler.

I'll try to have a look later but it fixes the issue馃憣

@tagliala tagliala deleted the fix-select-multiple-validation-feedback branch April 3, 2021 06:52
@tkrotoff
Copy link
Sponsor Contributor

tkrotoff commented Apr 9, 2021

This PR is not OK.

Screenshots for form-select

  • Chrome 89 desktop/Ubuntu

image

  • Chrome 88 desktop/Windows 10

image

  • Firefox 87 desktop/Windows 10

image

  • Chrome 89 desktop/macOS

image

  • Old Edge 17/42 Windows 10

image

The solution in Bootstrap 4 was to not display the feedback icon with custom-select (screenshot from Chrome 89 desktop/Ubuntu):

image

@tagliala
Copy link
Contributor Author

tagliala commented Apr 9, 2021

Hi, thanks for the report

I can replicate this issue also in BS4 4.6

image
image

Demo: https://jsfiddle.net/tagliala/L527xnrj/

I'm open to both fixes (remove the validation icon AND provide more space to take scrollbar into account)

@tkrotoff
Copy link
Sponsor Contributor

tkrotoff commented Apr 9, 2021

I can replicate this issue also in BS4 4.6

This is because you don't use custom-select

@tagliala
Copy link
Contributor Author

tagliala commented Apr 9, 2021

Ok, got it

this happens because

.form-select[multiple], .form-select[size]:not([size="1"]) {
    background-image: none;
}

in BS5 has less priority. I should investigate more

@tkrotoff
Copy link
Sponsor Contributor

tkrotoff commented Apr 9, 2021

provide more space to take scrollbar into account

Each browser and platform potentially has a different scrollbar with a different width.

Example with Firefox Ubuntu when you have a lot of options inside the select:
image

when you hover the scrollbar:
image

Chrome 89 mobile device (dev tools):
image

What about removing the scrollbar? Then you cannot scroll anymore :-) http://jsfiddle.net/zc4omj6q/1/ (from https://stackoverflow.com/a/11433927).

I think the least worst is to remove the feedback icon with select multiple. We still have the border color to give feedback to the user.

tagliala added a commit to tagliala/bootstrap that referenced this pull request Apr 13, 2021
Implementation provided in twbs#33411 does not take into account that some
Operating Systems may display a vertical scrollbar in the multiple
select field

This implementation will hide the validation icons from multiple select
fields, just like Bootstrap 4 does.

Fix: twbs#33591
mdo pushed a commit that referenced this pull request Apr 14, 2021
Implementation provided in #33411 does not take into account that some
Operating Systems may display a vertical scrollbar in the multiple
select field

This implementation will hide the validation icons from multiple select
fields, just like Bootstrap 4 does.

Fix: #33591
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.0
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

5 participants