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

Fixing floating button style horizontal padding #2113

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Fixing floating button style horizontal padding #2113

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
cd4fc6b
Fixing floatingButton horizontalPadding
daceball Jan 6, 2025
2d80675
Using alternative padding for floating with text only
daceball Jan 10, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions Sources/FluentUI_iOS/Components/Button/FluentButtonStyle.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,12 +97,11 @@ public struct FluentButtonStyle: SwiftUI.ButtonStyle {
private var edgeInsets: EdgeInsets {
let size = size
let horizontalPadding = ButtonTokenSet.horizontalPadding(style: style, size: size)
let fabAlternativePadding = ButtonTokenSet.fabAlternativePadding(size)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you check to see how this affects FAB controls with text? IIRC this alternate padding was used to ensure that buttons with text had the right amount of padding, which was slightly higher on the trailing edge.

Copy link
Author

@daceball daceball Jan 7, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Sure! In the Figma the trailing and leading padding have same values.

image

image

This is how the FAB with text looks after the update I made. Please let me know if it looks good and if there are other scenarios I should test. Thanks! (:

Screenshot 2025-01-07 at 12 25 18 PM

Copy link
Author

Choose a reason for hiding this comment

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

@mischreiber Could you confirm if the FAB with text example I added above looks correct? thanks!

Copy link
Contributor

Choose a reason for hiding this comment

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

There's something odd going on -- look at how much space there is between the right edge of the word Label and the actual bounding box that's being calculated. I'm investigating what's going on there now.

Copy link
Contributor

Choose a reason for hiding this comment

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

Confirmed: there appears to be a 4pt spacing between the actual label's bounding box, and the box being measured against.

image

Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a way we can figure out where those extra 4pts of spacing are coming from? Feels like a slightly painful API if users have to duplicate information of whether it has a label to us?

Copy link
Contributor

Choose a reason for hiding this comment

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

It's definitely an intentional design decision. The component looks unbalanced without the extra padding.

Copy link
Author

Choose a reason for hiding this comment

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

@mischreiber Thank you for confirming that this is by design. Could you please let me know if you agree with the fix I made by adding the isIconOnly parameter to the style init: FluentButtonStyle(style: .floatingSubtle, isIconOnly: true)? Thanks

Copy link
Contributor

Choose a reason for hiding this comment

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

@daceball is there a way to internally determine if it's an iconOnly/textOnly button, I agree with Anand that this is a slightly painful api then

Copy link
Author

Choose a reason for hiding this comment

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

@mischreiber Looks like ButtonStyle doesn’t have a built-in way to figure out if a button is icon-only or icon+text. In makeBody(configuration:), the configuration.label is just a View, so the style doesn’t get any extra info about whether there’s text or just an icon. We’d have to pass this info as a parameter (like in my last commit). I can think of the following approaches:

  • Pass a parameter indicating the type of floating button: icon-only or icon+text.
  • Create a new style type (e.g., style: .floatingSubtleIconOnly) and pick the right padding for each type.
  • Update the design and use the same padding for both icon-only and icon+text.

Let me know what you think about these options or if you have another idea to solve this bug. I'd really appreciate your input!

return EdgeInsets(
top: .zero,
leading: horizontalPadding,
bottom: .zero,
trailing: style.isFloating ? fabAlternativePadding : horizontalPadding
trailing: horizontalPadding
)
}

Expand Down
Loading