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

Add align-self: center to buttons for improved rendering in flex containers #34834

Merged
merged 3 commits into from Sep 7, 2021
Merged

Add align-self: center to buttons for improved rendering in flex containers #34834

merged 3 commits into from Sep 7, 2021

Conversation

zaidjawed
Copy link
Contributor

@zaidjawed zaidjawed commented Aug 28, 2021

Fixes #34833

When placing a button with size classes as direct child of a div with d-flex, size is not respected

<div class="d-flex flex-row">
  <button type="button" class="m-2 btn btn-primary">Default</button>
  <button type="button" class="m-2 btn btn-primary btn-sm">Small</button>
  <button type="button" class="m-2 btn btn-primary btn-md">Medium</button>
  <button type="button" class="m-2 btn btn-primary btn-lg">Large</button>
</div>

Produces
131220125-75cef342-d108-442d-811b-25a2ecb679a1

While

<div class="d-flex flex-row">
  <div>
    <button type="button" class="m-2 btn btn-primary">Default</button>
    <button type="button" class="m-2 btn btn-primary btn-sm">Small</button>
    <button type="button" class="m-2 btn btn-primary btn-md">Medium</button>
    <button type="button" class="m-2 btn btn-primary btn-lg">Large</button>
  </div>
</div>

Produces
131220163-0b761e9f-f518-4d68-bc99-43052cc18992

Solution:

According to W3Schools
the default value of align-items is stretch which might be causing this issue.

I simply added align-self to center to give a default value to center if the buttons are a direct child of flex.

Issue: #34833
Resolved button size not respected when direct child of div with d-flex by adding align-self to center in ".btn" class.
@zaidjawed zaidjawed requested a review from a team as a code owner August 28, 2021 15:26
@zaidjawed zaidjawed changed the title reolved #34833 resolved #34833 Aug 28, 2021
@zaidjawed zaidjawed changed the title resolved #34833 resolved : When placing a button with size classes as direct child of a div with d-flex, size is not respected Aug 28, 2021
@XhmikosR XhmikosR changed the title resolved : When placing a button with size classes as direct child of a div with d-flex, size is not respected Fix size not being respected when placing a button with size classes as direct child of a div with d-flex Aug 30, 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.

Feels safe to me, thanks for submitting this :)

Comment on lines 7 to 10

//button size not respected when direct child of div with d-flex #34833
align-self: center;

Copy link
Member

Choose a reason for hiding this comment

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

Please drop the line breaks and comments, this is not needed.

@mdo mdo added this to In progress in v5.1.1 via automation Sep 1, 2021
v5.1.1 automation moved this from In progress to Reviewer approved Sep 1, 2021
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.

I'm into it!

@mdo mdo removed this from Reviewer approved in v5.1.1 Sep 1, 2021
@mdo mdo added this to In progress in v5.1.2 via automation Sep 7, 2021
@mdo mdo merged commit 94c80ff into twbs:main Sep 7, 2021
v5.1.2 automation moved this from In progress to Done Sep 7, 2021
@mdo mdo changed the title Fix size not being respected when placing a button with size classes as direct child of a div with d-flex Add align-self: center to buttons for improved rendering in flex containers Sep 7, 2021
@wKovacs64
Copy link

This was a breaking change. Can't you just apply align-items-center on your flex container instead?

<div class="d-flex flex-row align-items-center">
  <button type="button" class="m-2 btn btn-primary">Default</button>
  <button type="button" class="m-2 btn btn-primary btn-sm">Small</button>
  <button type="button" class="m-2 btn btn-primary btn-md">Medium</button>
  <button type="button" class="m-2 btn btn-primary btn-lg">Large</button>
</div>

@719media
Copy link
Contributor

719media commented Oct 6, 2021

Agreed about breaking change. Yes, this makes buttons inside button groups not stretch to fill height under certain circumstances.

@andreas-venturini
Copy link

This broke most of the button styles in our application. I strongly suggest highlighting this change more prominently in the Changelog (possibly even mark as a breaking change). This is not what we would've expected of a minor patch release.

Anyway, keep up the good work ✌️

@ffoodd
Copy link
Member

ffoodd commented Oct 6, 2021

@wKovacs64 @719media @andreas-venturini Please open an issue to highlight broken cases, as it seems we missed them. If that's breaking too much, we might revert it.

@andreas-venturini
Copy link

@ffoodd done #35126

Thinking about this some more, I think you should consider reverting (I'm not disagreeing with this change per se, it might just be a bit too much for a minor patch release)

@amshkv
Copy link

amshkv commented Oct 8, 2021

This change was unexpected and broke the part of the interface where the parent was d-flex with align-items-start
Now we need to redefine all btn in this layout.
Especially unexpected for flex-column
image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
No open projects
v5.1.2
  
Done
Development

Successfully merging this pull request may close these issues.

button size not respected when direct child of div with d-flex
8 participants