Enable wrapping for elements in modal-footer

[MartijnCuppens]

Fixes #24800.

[mdo]

This is a no-go for me—we avoid universal selectors like this whenever possible.

[MartijnCuppens]

@mdo Why?

[andresgalante]

Hi @MartijnCuppens this PR explains it #25165

[MartijnCuppens]

I understand that we should use classes as much as possible. But in this case you don't know which elements are going to be placed in the .modal-footer.

Also right now the following code is used to place margins between the elements:

  // Easily place margin between footer elements
  > :not(:first-child) { margin-left: .25rem; }
  > :not(:last-child) { margin-right: .25rem; }

Imo this isn't more dogmatic as the use of the universal selector.

[mdo]

Oh word, hmm. Those are also universal selectors since they'll match every elements first before limiting to last/first children. Both are bad generally speaking because they take much longer for browsers to parse and apply.

I wonder if a mix of these existing styles and a .modal-footer-item would work? Or perhaps we just recommend spacing utils? A single universe selector is better than two though, too. Hmm...

[MartijnCuppens]

With spacing utils, there are some responsive issues (spacing between wrapped elements). Providing the .modal-footer-item class would be the best solution if we want to prevent the use of the universal selector. Downside: It must be added to all children of the .modal-footer and that would break all existing implementations of the modal.

[MartijnCuppens]

@mdo Have you made your mind up about this?

[MartijnCuppens]

We can now use the negative margin utility classes to fix this, but then still the 2 universal selectors (.modal-footer > :not(:first-child) & .modal-footer > :not(:last-child)) will remain, so it might be better to use the * here so that we only have one poorly performing selector instead of two. I've added a warning about the universal selector usage in this PR.

@mdo, your call if we continue with this.

[MartijnCuppens]

@twbs/css-review, this one is an older PR from me, but I think it's still relevant so I've added it to the v4.4.0 project

[mdo]

Sorry @MartijnCuppens! Looking at this again, my concern here is that this could cause unintended side effects for folks who have put other items in their .modal-footer (like text, custom elements, etc).

Should we instead limit the selector to > .btn?

[MartijnCuppens]

No problem, @mdo! We did already target all elements (not only .btn) with > :not(:first-child) and > :not(:last-child), so in this case it might be safer to use the *.

[mdo]

Oh, yeah, I'm dumb. This looks good then!

[anatolebeuzon]

Thanks for fixing this! 👏 👏

[karlsve]

Sorry for resurrecting the dead but I was just wondering if anyone had thought about how to handle multiple paddings with this fix?

customizations.scss

$modal-inner-padding: 5px 4px; // This will break the style as the resulting css will be concatenated

[MartijnCuppens]

@karlsve, that won't be possible with the current implementation, but could be fixed by splitting the variable into $modal-inner-padding-x and $modal-inner-padding-y variables. You may open a PR for this.

[karlsve]

As the $modal-inner-padding is only used by the .modal-body to actually define all its paddings, shouldn't this be using different variables anyways if there are calculations done?
This would cause developers to define the same value five times in the worst case, which isn't good either.

Another way would be to somehow force the calculation to be done on all elements of a list of values. I have to admit that I am not sure if SASS / SCSS supports this in any way.

It also seems to me like more people should be experiencing this issue as my team cannot be the only one which set the $modal-inner-padding like this in bootstrap 4.3.x.