Skip to content

Mhd/refactor compass action #286

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

Merged
merged 22 commits into from
Apr 3, 2023
Merged

Mhd/refactor compass action #286

merged 22 commits into from
Apr 3, 2023

Conversation

mhdostal
Copy link
Member

Closes #25, #266

This takes #281 and updates it a bit:

  • Add automaticallyHides view modifier and removes autoHide from the constructors
  • Removing autoHide allows the action to be a closure
  • Adds Viewpoint.withViewpoint(rotation:) extension for ease of viewpoint creation when only the rotation changed.
  • updates the MapView compass example to animate taps on the compass (rotation to 0).

it includes these changes from the original PR:

  • Adds an optional action parameter to the Compass initializer(s) that takes an async task. This allows users to perform animated heading changes. If no action is provided, behavior remains unchanged.
  • The name "action" is borrowed from Button.init(action:label:)

@mhdostal mhdostal requested a review from dfeinzimer March 31, 2023 17:28
@dfeinzimer
Copy link
Collaborator

It looks like removing the auto hide parameter broke a number of the tests.

Here's a prototyped fix for testHiddenWithAutoHideOff():

func testHiddenWithAutoHideOff() {
    let initialValue = 0.0
    let finalValue = 90.0
    var _viewpoint: Viewpoint? = makeViewpoint(rotation: initialValue)
    let viewpoint = Binding(get: { _viewpoint }, set: { _viewpoint = $0 })
    let compass = Compass(viewpoint: viewpoint).automaticallyHides(false)
    XCTAssertFalse((compass as? Compass)!.shouldHide)
    _viewpoint = makeViewpoint(rotation: finalValue)
    XCTAssertFalse((compass as? Compass)!.shouldHide)
}

@mhdostal mhdostal requested a review from dfeinzimer March 31, 2023 20:09
Copy link
Collaborator

@dfeinzimer dfeinzimer left a comment

Choose a reason for hiding this comment

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

Just the one change and this should be good to go.

Up to you if you want to wait for the moveCamera PR to merge or not.

@mhdostal mhdostal requested review from dfeinzimer and philium April 1, 2023 19:37
@mhdostal
Copy link
Member Author

mhdostal commented Apr 1, 2023

Up to you if you want to wait for the moveCamera PR to merge or not.

No, let's get this merged, it's been pending a while. Making the follow-up change can happen with other changes as well.

Copy link
Collaborator

@dfeinzimer dfeinzimer left a comment

Choose a reason for hiding this comment

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

Looking good, I think it should be good to go after these last few changes.

Copy link
Collaborator

@dfeinzimer dfeinzimer left a comment

Choose a reason for hiding this comment

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

Looks good!

@mhdostal mhdostal merged commit 7c63f93 into v.next Apr 3, 2023
@mhdostal mhdostal deleted the mhd/RefactorCompass_action branch April 3, 2023 16:27

## Behavior:

Whenever the map is not orientated North (non-zero bearing) the compass appears. When reset to north, it disappears. An initializer argument allows you to disable the auto-hide feature so that it always appears.
Whenever the map is not orientated North (non-zero bearing) the compass appears. When reset to north, it disappears. The `automaticallyHides` view modifier allows you to disable the auto-hide feature so that it is always displayed.
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this then be named more like how SwiftUI names modifiers like this?

maybe disableAutoHide(:)?

image

Copy link
Member Author

Choose a reason for hiding this comment

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

That would be OK. @dfeinzimer?

Copy link
Collaborator

Choose a reason for hiding this comment

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

#291

disableAutocorrection was deprecated in favor of autocorrectionDisabled so I'll follow the new scheme. e.g. autoHideDisabled.

@@ -41,14 +44,13 @@ public struct Compass: View {
/// direction toward true East, etc.).
/// - Parameters:
/// - heading: The heading of the compass.
/// - autoHide: A Boolean value that determines whether the compass
/// automatically hides itself when the heading is `0`.
/// - action: An action to perform when the compass is tapped.
Copy link
Contributor

Choose a reason for hiding this comment

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

some doc about when you need to specify this action might be helpful to the user. For example, does the compass set the heading to zero and call the action if I have an action set? What is the behavior when no action is set?

Copy link
Member Author

Choose a reason for hiding this comment

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

The default is to rotate the map view to zero, but without animation.

We'll update the doc.

Comment on lines +79 to +81
await proxy.setViewpoint(
viewpoint.withRotation(0),
duration: 0.25
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 lot of point to the compass component if the user has to animate to zero themselves?

Copy link
Member Author

Choose a reason for hiding this comment

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

It still draws the compass, which is non-trivial, automatically allows it to track map view rotation, and will allow tapping to perform an action. The default is to rotate the map view to zero, but without animation, as that requires a MapViewProxy. So yea, there is value in it, but we can re-evaluate the default behavior to see if we can include animation in a seamless way.

Copy link
Contributor

Choose a reason for hiding this comment

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

Good points. Thank you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants