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.

Sign up for GitHub

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

Jump to bottom

Enable wrapping for elements in modal-footer #25103

Merged
merged 2 commits into from Aug 16, 2019
Merged

Enable wrapping for elements in modal-footer #25103

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
30e91ca
Enable wrapping for elements in .modal-footer
MartijnCuppens Oct 30, 2018
cc0e9fa
Merge branch 'master' into modal-footer-fix
XhmikosR Aug 16, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
13 changes: 9 additions & 4 deletions scss/_modal.scss
View file
Open in desktop
Expand Up @@ -166,15 +166,20 @@
// Footer (for actions)
.modal-footer {
display: flex;
flex-wrap: wrap;
align-items: center; // vertically center
justify-content: flex-end; // Right align buttons with flex property because text-align doesn't work on flex items
padding: $modal-inner-padding;
padding: $modal-inner-padding - $modal-footer-margin-between / 2;
border-top: $modal-footer-border-width solid $modal-footer-border-color;
@include border-bottom-radius($modal-content-inner-border-radius);

// Easily place margin between footer elements
> :not(:first-child) { margin-left: .25rem; }
> :not(:last-child) { margin-right: .25rem; }
// Place margin between footer elements
// This solution is far from ideal because of the universal selector usage,
// but is needed to fix https://github.com/twbs/bootstrap/issues/24800
// stylelint-disable-next-line selector-max-universal
> * {
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

@mdo Why?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hi @MartijnCuppens this PR explains it #25165

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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...

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

@mdo Have you made your mind up about this?

margin: $modal-footer-margin-between / 2;
}
}

// Measure scrollbar width for padding body during modal show/hide
Expand Down
3 changes: 3 additions & 0 deletions scss/_variables.scss
View file
Open in desktop
Expand Up @@ -933,6 +933,9 @@ $badge-border-radius: $border-radius !default;
// Padding applied to the modal body
$modal-inner-padding: 1rem !default;

// Margin between elements in footer, must be lower than or equal to 2 * $modal-inner-padding
$modal-footer-margin-between: .5rem !default;

$modal-dialog-margin: .5rem !default;
$modal-dialog-margin-y-sm-up: 1.75rem !default;

Expand Down