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

fix(fab-button): position is correct with custom sizes #28195

Merged
merged 8 commits into from
Sep 28, 2023
Merged

Conversation

liamdebeasi
Copy link
Contributor

@liamdebeasi liamdebeasi commented Sep 19, 2023

Issue number: resolves #22564


What is the current behavior?

Changing the size of the FAB button causes it to be positioned incorrectly. This was happening because we set position values based on the assumption that the default FAB button would always be 56px x 56px.

What is the new behavior?

  • FAB and FAB List positioning is now computed based on intrinsic size

Does this introduce a breaking change?

  • Yes
  • No

Other information

Dev build: 7.4.1-dev.11695395641.14897417

@github-actions github-actions bot added the package: core @ionic/core package label Sep 19, 2023
@include margin(($fab-size - $fab-small-size) * 0.5);
@include margin($fab-button-small-margin);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was always 8px, so I just made it its own variable

$fab-size: 56
$fab-small-size: 40

(56 - 40) * 0.5 = 16 * 0.5 = 8

Comment on lines 17 to 16
min-height: $fab-size;
min-width: $fab-small-size + ($fab-button-small-margin * 2);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These evaluate to the same value. However, the purpose of this is so the list is aligned with the parent FAB button. I decided to make this a bit more verbose so its clear where 56px is coming from. I originally thought that this was a bug since small FAB buttons can be 40px until I realized we were trying to account for the small FAB button margin too.

@liamdebeasi liamdebeasi changed the title exploration fix(fab-button): position is correct with custom sizes Sep 19, 2023
@liamdebeasi liamdebeasi marked this pull request as ready for review September 22, 2023 15:22
@liamdebeasi liamdebeasi requested review from brandyscarney and a team September 22, 2023 15:22
Copy link
Contributor

@averyjohnston averyjohnston left a comment

Choose a reason for hiding this comment

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

I like how you structured the test here, it's very elegant!

@liamdebeasi liamdebeasi added this pull request to the merge queue Sep 28, 2023
Merged via the queue into main with commit eb41b55 Sep 28, 2023
@liamdebeasi liamdebeasi deleted the FW-4930 branch September 28, 2023 15:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package: core @ionic/core package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

bug: .fab-horizontal-center class is not responsive
3 participants