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

Improve FloatingButton styling. #4060

Merged
merged 2 commits into from Sep 15, 2022

Conversation

MaximAlien
Copy link
Contributor

@MaximAlien MaximAlien commented Aug 9, 2022

Description

  • Remove shadow that is applied to FloatingButton to be aligned with other stylable views and to improve performance (shadow usage leads to such warning: The layer is using dynamic shadows which are expensive to render. If possible try setting 'shadowPath', or pre-rendering the shadow into an image and putting it under the layer.).

  • Add the ability apply type and cornerRadius while creating FloatingButton.

@MaximAlien MaximAlien added the UI Work related to visual components, Android Auto, Camera, 3D, voice, etc. label Aug 9, 2022
@MaximAlien MaximAlien added this to the v2.8 milestone Aug 9, 2022
@MaximAlien MaximAlien requested a review from a team August 9, 2022 15:28
@MaximAlien MaximAlien self-assigned this Aug 9, 2022
@MaximAlien MaximAlien marked this pull request as draft August 9, 2022 15:28
@mapbox-github-ci-issues-public-1

Breaking Changes in MapboxNavigation

Breaking API Changes

FloatingButton

  • removed source.lang.swift.decl.function.method.class: rounded(image:selectedImage:size:) in FloatingButton

@@ -603,6 +604,7 @@ class ViewController: UIViewController {
animations: {
navigationViewController.navigationView.wayNameView.alpha = 1.0
navigationViewController.navigationView.floatingStackView.alpha = 1.0
navigationViewController.navigationView.speedLimitView.alpha = 1.0
Copy link
Contributor Author

@MaximAlien MaximAlien Aug 9, 2022

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.

Copy link
Contributor

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.

func update() {
let wasHidden = isHidden
isHidden = !canDraw
setNeedsDisplay()
if canDraw && !wasHidden {
blinkIn()
}
}

Copy link
Contributor Author

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 dismissing NavigationViewController).

@MaximAlien MaximAlien marked this pull request as ready for review August 9, 2022 16:58
@@ -125,10 +118,6 @@ open class NavigationView: UIView {
return stackView
}()

lazy var overviewButton = FloatingButton.rounded(image: Images.overview)
Copy link
Contributor Author

@MaximAlien MaximAlien Aug 9, 2022

Choose a reason for hiding this comment

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

IMHO these floating buttons should not be in NavigationView anymore as it can be used not only in active navigation.

Copy link
Contributor

Choose a reason for hiding this comment

The 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 NavigationView?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is actually another ResumeButton that currently resides in NavigationView, I haven't made any changes to it in scope of this PR, but it'd make sense to move it outside of NavigationView as well, because it's only active-navigation related UI component, and NavigationView is meant to be used in free-drive as well.

The reason I keep it here is because it's likely to be removed/deprecated altogether to match Android behavior.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated style:

Screen Shot 2022-09-06 at 2 51 43 PM

Screen Shot 2022-09-06 at 3 00 31 PM

@mapbox-github-ci-issues-public-1

Breaking Changes in MapboxNavigation

Breaking API Changes

FloatingButton

  • removed source.lang.swift.decl.function.method.class: rounded(image:selectedImage:size:) in FloatingButton

@@ -44,13 +44,6 @@ open class NavigationView: UIView {
static let buttonSpacing: CGFloat = 8.0
}

private enum Images {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Was moved to UIImage extension.

@@ -603,6 +604,7 @@ class ViewController: UIViewController {
animations: {
navigationViewController.navigationView.wayNameView.alpha = 1.0
navigationViewController.navigationView.floatingStackView.alpha = 1.0
navigationViewController.navigationView.speedLimitView.alpha = 1.0
Copy link
Contributor

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.

func update() {
let wasHidden = isHidden
isHidden = !canDraw
setNeedsDisplay()
if canDraw && !wasHidden {
blinkIn()
}
}

Comment on lines +52 to +54
class var defaultBorderWidth: CGFloat {
1 / UIScreen.main.scale
}
Copy link
Contributor

Choose a reason for hiding this comment

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

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)
Copy link
Contributor

Choose a reason for hiding this comment

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

This is removing shadow completely, right? Would users expect that?

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@@ -125,10 +118,6 @@ open class NavigationView: UIView {
return stackView
}()

lazy var overviewButton = FloatingButton.rounded(image: Images.overview)
Copy link
Contributor

Choose a reason for hiding this comment

The 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 NavigationView?

@MaximAlien MaximAlien force-pushed the maxim/make-floating-button-customizable branch from bfafdfd to 7064152 Compare August 25, 2022 00:48
@MaximAlien MaximAlien force-pushed the maxim/make-floating-button-customizable branch from 7064152 to 53ac7cd Compare September 6, 2022 21:11
@mapbox-github-ci-issues-public-1

Breaking Changes in MapboxNavigation

Breaking API Changes

FloatingButton

  • removed source.lang.swift.decl.function.method.class: rounded(image:selectedImage:size:) in FloatingButton

@mapbox-github-ci-issues-public-1

Breaking Changes in MapboxNavigation

Breaking API Changes

FloatingButton

  • removed source.lang.swift.decl.function.method.class: rounded(image:selectedImage:size:) in FloatingButton

@MaximAlien MaximAlien force-pushed the maxim/make-floating-button-customizable branch from 0608050 to 9f56ffc Compare September 6, 2022 23:08
@mapbox-github-ci-issues-public-1

Breaking Changes in MapboxNavigation

Breaking API Changes

FloatingButton

  • removed source.lang.swift.decl.function.method.class: rounded(image:selectedImage:size:) in FloatingButton

@MaximAlien MaximAlien requested review from a team, 1ec5 and chezzdev September 6, 2022 23:36
@MaximAlien MaximAlien force-pushed the maxim/make-floating-button-customizable branch from 9f56ffc to 1eb5f68 Compare September 13, 2022 23:56
…ling that is similar to other UI components.
@MaximAlien MaximAlien force-pushed the maxim/make-floating-button-customizable branch from 1eb5f68 to 8646e78 Compare September 14, 2022 19:23
@mapbox-github-ci-issues-public-1

Breaking Changes in MapboxNavigation

Breaking API Changes

FloatingButton

  • removed source.lang.swift.decl.function.method.class: rounded(image:selectedImage:size:) in FloatingButton

Copy link
Contributor

@1ec5 1ec5 left a comment

Choose a reason for hiding this comment

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

Let’s mention the border removal in the changelog. Even though it doesn’t affect the public API, it is a user-visible change that may necessitate tweaks to some custom UI styles. A public FloatingButton method has also been renamed to add the type and cornerRadius parameters.

@MaximAlien MaximAlien removed this from the v2.8 milestone Sep 15, 2022
@MaximAlien MaximAlien added this to the v2.9 milestone Sep 15, 2022
@MaximAlien MaximAlien merged commit c2d6233 into main Sep 15, 2022
@MaximAlien MaximAlien deleted the maxim/make-floating-button-customizable branch September 15, 2022 00:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
UI Work related to visual components, Android Auto, Camera, 3D, voice, etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants