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
Improve FloatingButton
styling.
#4060
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -44,16 +44,30 @@ open class FloatingButton: Button { | |
- parameter image: The `UIImage` of this button. | ||
- parameter selectedImage: The `UIImage` of this button when selected. | ||
- parameter size: The size of this button, or `FloatingButton.buttonSize` if this argument is not specified. | ||
- parameter type: `UIButton` type. Defaults to `.custom`. | ||
- parameter cornerRadius: Corner radius of the button. | ||
|
||
- returns: `FloatingButton` instance. | ||
*/ | ||
public class func rounded<T: FloatingButton>(image: UIImage? = nil, | ||
selectedImage: UIImage? = nil, | ||
size: CGSize = FloatingButton.buttonSize) -> T { | ||
let button = T.init(type: .custom) | ||
size: CGSize = FloatingButton.buttonSize, | ||
type: UIButton.ButtonType = .custom, | ||
cornerRadius: CGFloat = FloatingButton.buttonSize.width / 2.0) -> T { | ||
let button = T.init(type: type) | ||
button.translatesAutoresizingMaskIntoConstraints = false | ||
button.constrainedSize = size | ||
button.setImage(image, for: .normal) | ||
if let selected = selectedImage { button.setImage(selected, for: .selected) } | ||
button.applyDefaultCornerRadiusShadow(cornerRadius: size.width / 2) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is removing shadow completely, right? Would users expect that? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a tradeoff for performance as noted above. I think it could look like an intentional design change, as opposed to a bug – not dissimilar to changes in Apple software over the years. But the main thing is to make sure the border is sufficiently discernible. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think two major reasons for this change is consistency and only partly performance. Previously we've shown shadow, border or no border in views where consistency is expected (top and bottom banners, resume button, floating button). With shadow removal we make look more consistent and customizable to bring new UI updates for next iteration of drop-in. |
||
button.layer.cornerRadius = cornerRadius | ||
button.imageView?.contentMode = .scaleAspectFit | ||
|
||
if let image = image { | ||
button.setImage(image, for: .normal) | ||
} | ||
|
||
if let selectedImage = selectedImage { | ||
button.setImage(selectedImage, for: .selected) | ||
} | ||
|
||
return button | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -44,13 +44,6 @@ open class NavigationView: UIView { | |
static let buttonSpacing: CGFloat = 8.0 | ||
} | ||
|
||
private enum Images { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Was moved to |
||
static let overview = UIImage(named: "overview", in: .mapboxNavigation, compatibleWith: nil)!.withRenderingMode(.alwaysTemplate) | ||
static let volumeUp = UIImage(named: "volume_up", in: .mapboxNavigation, compatibleWith: nil)!.withRenderingMode(.alwaysTemplate) | ||
static let volumeOff = UIImage(named: "volume_off", in: .mapboxNavigation, compatibleWith: nil)!.withRenderingMode(.alwaysTemplate) | ||
static let feedback = UIImage(named: "feedback", in: .mapboxNavigation, compatibleWith: nil)!.withRenderingMode(.alwaysTemplate) | ||
} | ||
|
||
var compactConstraints = [NSLayoutConstraint]() | ||
var regularConstraints = [NSLayoutConstraint]() | ||
|
||
|
@@ -125,10 +118,6 @@ open class NavigationView: UIView { | |
return stackView | ||
}() | ||
|
||
lazy var overviewButton = FloatingButton.rounded(image: Images.overview) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. IMHO these floating buttons should not be in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we then move its management (showing, hiding, etc.) to the view controller? Seems like an extra knowledge for the view, if it doesn't contain the buttons. Also, could it be a breaking change for the users of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I do not consider this as a breaking change because all components are internal and not exposed to the user. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There is actually another The reason I keep it here is because it's likely to be removed/deprecated altogether to match Android behavior. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
lazy var muteButton = FloatingButton.rounded(image: Images.volumeUp, selectedImage: Images.volumeOff) | ||
lazy var reportButton = FloatingButton.rounded(image: Images.feedback) | ||
|
||
var floatingButtonsPosition: MapOrnamentPosition = .topTrailing { | ||
didSet { | ||
setupConstraints() | ||
|
@@ -143,7 +132,7 @@ open class NavigationView: UIView { | |
} | ||
} | ||
|
||
lazy var resumeButton: ResumeButton = .forAutoLayout() | ||
lazy var resumeButton: ResumeButton = .forAutoLayout(hidden: true) | ||
|
||
var wayNameViewLayoutGuide: UILayoutGuide? { | ||
didSet { | ||
|
@@ -226,7 +215,6 @@ open class NavigationView: UIView { | |
|
||
func commonInit() { | ||
DayStyle().apply() | ||
floatingButtons = [overviewButton, muteButton, reportButton] | ||
setupViews() | ||
setupConstraints() | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -49,6 +49,14 @@ open class Style: NSObject { | |
UITraitCollection(userInterfaceIdiom: .pad), | ||
]) | ||
|
||
class var defaultBorderWidth: CGFloat { | ||
1 / UIScreen.main.scale | ||
} | ||
Comment on lines
+52
to
+54
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The original purpose of the shadow was to ensure contrast with a map background that could be the exact same color. A border can accomplish the same contrast, though we should do some field-testing to verify that the border is thick and dark enough to make the button discernible in a sunny, washed-out lighting condition. |
||
|
||
class var defaultCornerRadius: CGFloat { | ||
10.0 | ||
} | ||
|
||
/** | ||
Applies the style for all changed properties. | ||
*/ | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just improves overall animation when presenting and dismissing
NavigationViewController
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this compatible with SpeedLimitView’s built-in behavior to hide when there’s no content and to blink when the speed limit updates? The blinking operates on the view’s layer’s opacity.
mapbox-navigation-ios/Sources/MapboxNavigation/SpeedLimitView.swift
Lines 114 to 121 in 793a34f
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I did a couple of tests and
SpeedLimitView
blinking occurs as expected whenever speed limit is changed (after showing or dismissingNavigationViewController
).