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

Example for navigation simulation #1328

Merged
merged 11 commits into from Jun 20, 2022
Merged

Example for navigation simulation #1328

merged 11 commits into from Jun 20, 2022

Conversation

maios
Copy link
Contributor

@maios maios commented May 11, 2022

MAPSIOS-167 Create a simulation for a navigation with vanishing routes effect.

Simulator.Screen.Recording.-.iPhone.13.mini.-.2022-06-13.at.15.05.32.mp4

Please note that there is something in progress calculation of Turf so it seems that the progress goes ahead of the movement, this also happens on Android. We will be looking into this separately.

Pull request checklist:

  • Describe the changes in this PR, especially public API changes.
  • Add a changelog entry to to bottom of the relevant section (typically the ## main heading near the top).

@maios maios mentioned this pull request May 31, 2022
5 tasks
@maios maios force-pushed the example/navigation-simulator branch from a3cf04e to ca4ba2c Compare June 13, 2022 11:51
@maios maios marked this pull request as ready for review June 13, 2022 11:51
@maios maios changed the title [WIP] Example for navigation simulation Example for navigation simulation Jun 13, 2022
@maios maios requested review from OdNairy and evil159 June 13, 2022 12:11
Comment on lines 35 to 36
init(mapView: MapView, route: LineString) {
self.viewport = mapView.viewport
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 use Viewport as an argument here as we are not using mapView anymore?

mapView.topAnchor.constraint(equalTo: view.safeAreaLayoutGuide.topAnchor),
mapView.leftAnchor.constraint(equalTo: view.safeAreaLayoutGuide.leftAnchor),
mapView.bottomAnchor.constraint(equalTo: view.safeAreaLayoutGuide.bottomAnchor),
mapView.rightAnchor.constraint(equalTo: view.safeAreaLayoutGuide.rightAnchor),
Copy link
Contributor

Choose a reason for hiding this comment

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

Constraining the map to safe area makes it look worse than the rest of examples:

Navigation Simulation Display a map view

Copy link
Contributor Author

Choose a reason for hiding this comment

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

interesting, we should actually constraint or bind the map to the safe area and not the whole view controller, since it is configured for its contents to be extended under the navigation bar -> we can't see the top of the map as Andrew pointed out here #1225 (comment)
I think the way navigation bar looks like that when view is constrained to navigation bar is because of the way we configure UINavigationBarAppearance for iOS 13+

this snippet fixed the problem though, wdyt?

        if #available(iOS 13.0, *) {
            let navigationBarAppearance = UINavigationBarAppearance()
            navigationBarAppearance.configureWithOpaqueBackground()

            appearance.scrollEdgeAppearance = navigationBarAppearance
            appearance.compactAppearance = navigationBarAppearance
            appearance.standardAppearance = navigationBarAppearance
        }

Copy link
Contributor

Choose a reason for hiding this comment

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

I think extending the map under the bars and making it fill the whole screen makes it looks like it belongs on iOS(the map is visible under the translucent bars). I checked how apple handles this in their Maps app, they use the globe view on lower zoom levels that eliminates this issue. Google Maps seems to both use opaque bars and have a part of the map behind them(obstructing a portion of the map behind them, of course).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

for the sake of example I think it makes more sense to display the full map without any part that is hidden, since we don't really have anything to show/hide navigation bar so it looks weird if a part of the map is not visible.

Full Safe-area
Simulator Screen Shot - iPhone 11 - 2022-06-16 at 12 10 38 Simulator Screen Shot - iPhone 11 - 2022-06-16 at 12 10 57

Comment on lines +140 to +143
Exp(.product) {
7.0
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.

I'm not quite sure if I understand this correctly, but shouldn't it translate to something like 7.0 * 1.0 - given that .product - "Returns the product of the inputs"?

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 think explicitly putting one here is for the sake of readiblity in its string representation :-D we could instead do Exp(.literal) { value } but I think we should keep this to make it consistent with Android.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see, having a column of just numbers, indeed, will make it hard to visually parse. However this more verbose notation seems to be a bit superfluous.
I wonder would it make it more readable if zoom level and raw line width value would be grouped on a single line, creating sort of a switch statement:

casingLayer.lineWidth = .expression(
    Exp(.interpolate) {
        Exp(.exponential) {
            1.5
        }
        Exp(.zoom)
        10.0; 7.0
        14.0; 10.5
        16.5; 15.5
        19.0; 24.0
        22.0; 29.0
    }
)

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 too think the syntax of iOS Expression could be improved. At the moment it can take up too many lines of code and provide little readability :-D would be nice to have a team discussion on this and propose new or addition way for Expression.

Apps/Examples/Examples/Models/Examples.swift Outdated Show resolved Hide resolved
@maios maios force-pushed the example/navigation-simulator branch from 844c214 to 07206ff Compare June 20, 2022 12:11
@maios maios merged commit 8a77c07 into main Jun 20, 2022
@maios maios deleted the example/navigation-simulator branch June 20, 2022 12:36
persidskiy pushed a commit that referenced this pull request Feb 22, 2023
* Add jazzy-theme as a submodule

* Correct working directory for build-docs

* Point studio preview Podfile to the internal repo

* Top-level podspec

* Private Github token for api compatibility check

* Fix paths in generate-maps-docs

* Install python dependencies for create-draft-release

* Fix path in create-github-draft-release

* Remove extranneous release-precheck requirement

* Use local maps sdk to validate studio preview
OdNairy pushed a commit that referenced this pull request Apr 5, 2023
* Add jazzy-theme as a submodule

* Correct working directory for build-docs

* Point studio preview Podfile to the internal repo

* Top-level podspec

* Private Github token for api compatibility check

* Fix paths in generate-maps-docs

* Install python dependencies for create-draft-release

* Fix path in create-github-draft-release

* Remove extranneous release-precheck requirement

* Use local maps sdk to validate studio preview
OdNairy pushed a commit that referenced this pull request Aug 22, 2023
* Add jazzy-theme as a submodule

* Correct working directory for build-docs

* Point studio preview Podfile to the internal repo

* Top-level podspec

* Private Github token for api compatibility check

* Fix paths in generate-maps-docs

* Install python dependencies for create-draft-release

* Fix path in create-github-draft-release

* Remove extranneous release-precheck requirement

* Use local maps sdk to validate studio preview
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.

None yet

4 participants