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

Hide validation icons from multiple selects #33598

Merged
merged 1 commit into from Apr 14, 2021

Conversation

tagliala
Copy link
Contributor

@tagliala tagliala commented Apr 9, 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

Demo fiddles

BS4: https://jsfiddle.net/tagliala/y1t4cuqg/show
BS5: https://jsfiddle.net/tagliala/s45v1hLy/show

Before (Win 10 / Chrome 89)

Main

image

5.0.0.beta3

image

After (Win 10 / Chrome 89)

image

Bootstrap 4 vs Bootstrap 5

image

@tagliala
Copy link
Contributor Author

tagliala commented Apr 9, 2021

Hi @tkrotoff this mimics BS4 approach, any chance to take a look at this PR?

Copy link
Sponsor Contributor

@tkrotoff tkrotoff left a comment

Choose a reason for hiding this comment

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

What about:

  select { <=====
    @include form-validation-state-selector($state) {
      &[multiple],
      &[size]:not([size="1"]) {
        padding-right: $form-select-padding-x;
        background-image: none;
      }
    }
  }

instead of

  .form-select {
    @include form-validation-state-selector($state) {
      &[multiple],
      &[size]:not([size="1"]) {
        padding-right: $form-select-padding-x;
        background-image: none;
      }
    }
  }

This way it fixes also your comment: #33411 (comment)

@mdo
Copy link
Member

mdo commented Apr 9, 2021

There should be no difference in moving from .form-select to select because we only have one class for this in v5. I’d stick with the class selector. I’m also good with moving in this direction to hide the icon again.

@tkrotoff
Copy link
Sponsor Contributor

tkrotoff commented Apr 9, 2021

I see several solutions depending on whether #33411 (comment) should be treated or not (is Bootstrap 5 supposed to work if the user writes select.form-control instead of .form-select?)

Modify scss/mixins/_forms.scss

Modifying _forms.scss has the drawback to have almost the same code inside _form-select.scss but the code can be factorized (something like remove-select-multiple-background-image())

Modify scss/forms/_form-select.scss

if we modify _form-select.scss this way, we don't need the same code inside _forms.scss

The problem with !important is that the user cannot set its own background-image to the select/.form-select.

I'm not a affiliated with Bootstrap, just my 2€

@tagliala
Copy link
Contributor Author

tagliala commented Apr 9, 2021

given @ffoodd's comment at #33411 (comment), would it be a better solution to only apply style to .form-select without multiple or with [size="1"]? Is there an edge case that I'm missing?

      @if $enable-validation-icons {
        &:not([multiple]),
        &[size="1"] {
          padding-right: $form-select-feedback-icon-padding-end;
          background-image: escape-svg($form-select-indicator), escape-svg($icon);
          background-position: $form-select-bg-position, $form-select-feedback-icon-position;
          background-size: $form-select-bg-size, $form-select-feedback-icon-size;
        }
      }

@tkrotoff
Copy link
Sponsor Contributor

tkrotoff commented Apr 10, 2021

@tagliala I've tested &:not([multiple]), &[size="1"] (nicer approach than my suggestions!) => does not work in all cases:

image

This works (but does not solve #33411 (comment)):

  .form-select {
      ...

      @if $enable-validation-icons {
        &:not([multiple]):not([size]),
        &[size="1"] {
          padding-right: $form-select-feedback-icon-padding-end;
          background-image: escape-svg($form-select-indicator), escape-svg($icon);
          background-position: $form-select-bg-position, $form-select-feedback-icon-position;
          background-size: $form-select-bg-size, $form-select-feedback-icon-size;
        }
      }

      ...
    }

image

Code to test:

<h2>select form-control (supported by Bootstrap 4, should it be supported by Bootstrap 5?)</h2>

<div class="mb-3">
  <select class="form-control is-valid">
    <option>1</option>
    <option>2</option>
    <option>3</option>
    <option>4</option>
    <option>5</option>
  </select>
</div>

<h2>form-select</h2>

<div class="mb-3">
  <select class="form-select is-valid">
    <option>1</option>
    <option>2</option>
    <option>3</option>
    <option>4</option>
    <option>5</option>
  </select>
</div>

<h2>form-select multiple</h2>

<div class="mb-3">
  <select class="form-select is-valid" multiple>
    <option>1</option>
    <option>2</option>
    <option>3</option>
    <option>4</option>
    <option>5</option>
  </select>
</div>

<h2>form-select size="1"</h2>

<div class="mb-3">
  <select class="form-select is-valid" size="1">
    <option>1</option>
    <option>2</option>
    <option>3</option>
    <option>4</option>
    <option>5</option>
  </select>
</div>

<h2>form-select size="2"</h2>

<div class="mb-3">
  <select class="form-select is-valid" size="2">
    <option>1</option>
    <option>2</option>
    <option>3</option>
    <option>4</option>
    <option>5</option>
  </select>
</div>

Screenshot with untouched v5 beta3:

image

btw you will notice that Bootstrap does not respect size="2" (it displays a bit more than 2 lines), screenshot with regular select size="2":

image

@tagliala
Copy link
Contributor Author

tagliala commented Apr 11, 2021

Thanks, of course I was missing something

btw, &:not([multiple]):not([size]), &[size="1"] { does not cover multiple size="1"

image

          <form>
            <code>form-select</code>
            <div class="mb-3">
              <select class="form-select is-valid">
                <option>1</option>
                <option>2</option>
                <option>3</option>
              </select>
            </div>
            <code>form-select size="1"</code>
            <div class="mb-3">
              <select class="form-select is-valid" size="1">
                <option>1</option>
                <option>2</option>
                <option>3</option>
              </select>
            </div>
            <code>form-select size="2"</code>
            <div class="mb-3">
              <select class="form-select is-valid" size="2">
                <option>1</option>
                <option>2</option>
                <option>3</option>
              </select>
            </div>
            <code>form-select multiple</code>
            <div class="mb-3">
              <select class="form-select is-valid" multiple>
                <option>1</option>
                <option>2</option>
                <option>3</option>
              </select>
            </div>
            <code>form-select multiple size="1"</code>
            <div class="mb-3">
              <select class="form-select is-valid" multiple size="1">
                <option>1</option>
                <option>2</option>
                <option>3</option>
              </select>
            </div>
            <code>form-select multiple size="2"</code>
            <div class="mb-3">
              <select class="form-select is-valid" multiple size="2">
                <option>1</option>
                <option>2</option>
                <option>3</option>
              </select>
            </div>
          </form>

The correct inversion should be

&:not([multiple]):not([size]),
&:not([multiple])[size="1"] {

Bootstrap 4 vs Bootstrap 5

image

@tkrotoff
Copy link
Sponsor Contributor

tkrotoff commented Apr 11, 2021

👍 You're right about multiple size="1": it displays a scrollbar and thus there should be no feedback icon and no indicator icon.

image

@tagliala tagliala force-pushed the bugfix/hide-validation-for-multiple-selects branch from 26a26c2 to 88f5834 Compare April 11, 2021 15:02
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
@tagliala tagliala force-pushed the bugfix/hide-validation-for-multiple-selects branch from 88f5834 to 491095a Compare April 13, 2021 08:59
@tagliala tagliala marked this pull request as ready for review April 13, 2021 09:02
@tagliala tagliala requested a review from a team as a code owner April 13, 2021 09:02
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.

Wow, appreciate the effort in this PR! The selector specificity isn't my favorite, but it solves the problem well. I think we can ship this.

@mdo mdo added this to Inbox in v5.0.0 via automation Apr 13, 2021
v5.0.0 automation moved this from Inbox to Approved Apr 13, 2021
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.

Agreed, thanks for the discussions and researches!

@mdo mdo merged commit 6fe75df into twbs:main Apr 14, 2021
v5.0.0 automation moved this from Approved to Done Apr 14, 2021
@tagliala tagliala deleted the bugfix/hide-validation-for-multiple-selects branch April 14, 2021 07:00
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.

form-select[multiple] should not display any indicator/icon
4 participants