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

Add upcoming maneuver arrow on the route line #934

Merged
merged 1 commit into from
Jun 6, 2018

Conversation

Guardiola31337
Copy link
Contributor

@Guardiola31337 Guardiola31337 commented May 14, 2018

upcoming_maneuver_arrow

TODO

  • Unit tests
  • Make the arrow’s color styleable instead of always red/white
  • 👀 behavior when off route
  • Add Expressions so that the arrow looks better under different zoom levels

@Guardiola31337 Guardiola31337 added ⚠️ DO NOT MERGE PR should not be merged! platform parity Required to keep on par with iOS. feature New feature request. navigation-ui labels May 14, 2018
@Guardiola31337 Guardiola31337 added this to the 0.14.0 milestone May 14, 2018
@Guardiola31337 Guardiola31337 self-assigned this May 14, 2018
@Cjballard-zz
Copy link

@Guardiola31337 Looking good – 2 quick design thoughts. I think the maneuver arrow might stand out more if it sat on top of the street name – right now both are kind of muddled when they're superimposed on each other. Unless consensus has been to leave the arrow underneath the street names? And I think having a slightly slimmer grey or black stroke on the arrow instead of the red might help make the arrow stand out a little more. What are your thoughts?

@Guardiola31337
Copy link
Contributor Author

@cjballard thanks for the feedback!

Unless consensus has been to leave the arrow underneath the street names?

How do you implement this @mapbox/navigation-ios?

I think the maneuver arrow might stand out more if it sat on top of the street name

In any case, even if we're adding the arrow underneath in the iOS side, if you think that from a design point of view the opposite makes more sense, we should change it in both platforms.

I think having a slightly slimmer grey or black stroke on the arrow instead of the red might help make the arrow stand out a little more.

The red was only for testing 😅
Also as indicated in OP, as part of this PR we're going to add 👇

Make the arrow’s color styleable instead of always red/white

BTW, great idea! We can definitely use those colors by default. Could you share the hex colors you were thinking so we can test them out and 👀 how arrows look?

@Cjballard-zz
Copy link

@Guardiola31337 Thanks for calling out – I like the idea of making it style-able. I think we should stack the arrow on top of the road name for driver clarity. (I think this is behavior on iOS side as well). Can we make the stroke our standard dark blue/gray (#273d56) but slightly slimmer and see what that looks like?

@1ec5
Copy link
Contributor

1ec5 commented May 17, 2018

Unless consensus has been to leave the arrow underneath the street names?

On iOS, the arrow goes on the very top of the map, above labels, shields, exit numbers, and POI icons alike. mapbox/mapbox-navigation-ios#397 (comment) shows what can easily happen to the arrow if we allow anything to appear atop it.

@Guardiola31337
Copy link
Contributor Author

How about now @cjballard?

arrow_maneuver

BTW our dark blue/gray color is #2D3F53 👀

<color name="mapbox_navigation_view_color_primary_dark">#2D3F53</color>

cc @1ec5

@Guardiola31337 Guardiola31337 force-pushed the pg-604-upcoming-maneuver-arrow branch 2 times, most recently from 6dd2300 to fabf549 Compare May 17, 2018 16:10
@1ec5
Copy link
Contributor

1ec5 commented May 17, 2018

Ideally, the arrowhead’s size would vary based on the zoom level so it looks more prominent at the highest zoom levels.

@Guardiola31337
Copy link
Contributor Author

Ideally, the arrowhead’s size would vary based on the zoom level so it looks more prominent at the highest zoom levels.

Yeah we should add Expressions around the arrow. Thanks for the heads up!

Editing OP to include that TODO.

@Guardiola31337 Guardiola31337 force-pushed the pg-604-upcoming-maneuver-arrow branch from 6a86792 to e4fbba8 Compare May 21, 2018 14:15
@Guardiola31337
Copy link
Contributor Author

Ideally, the arrowhead’s size would vary based on the zoom level so it looks more prominent at the highest zoom levels.

BTW, what expression are you using on iOS @1ec5? Saw that mapbox/mapbox-navigation-ios#1076 landed 🎉 some time ago.

@Guardiola31337 Guardiola31337 force-pushed the pg-604-upcoming-maneuver-arrow branch 2 times, most recently from 47e273c to 6e1912e Compare May 21, 2018 19:10
@1ec5
Copy link
Contributor

1ec5 commented May 21, 2018

Ideally, the arrowhead’s size would vary based on the zoom level so it looks more prominent at the highest zoom levels.

BTW, what expression are you using on iOS @1ec5? Saw that mapbox/mapbox-navigation-ios#1076 landed 🎉 some time ago.

https://github.com/mapbox/mapbox-navigation-ios/blob/6cd26dc9d428449df838cd7067d2e2d8298277f1/MapboxNavigation/NavigationMapView.swift#L630
https://github.com/mapbox/mapbox-navigation-ios/blob/c7d936db261e8a6fc49c0c095060fb19b371ba34/MapboxNavigation/Constants.swift#L11-L17

This is equivalent to the following style JSON:

{
  "icon-size": [
    "interpolate",
    ["linear"],
    ["zoom"],
    10, ["*", 8, 0.12],
    13, ["*", 9, 0.12],
    16, ["*", 11, 0.12],
    19, ["*", 22, 0.12],
    22, ["*", 28, 0.12]
  ]
}

@Guardiola31337 Guardiola31337 force-pushed the pg-604-upcoming-maneuver-arrow branch from 86ddfca to 3fc5517 Compare May 23, 2018 13:36
@Guardiola31337
Copy link
Contributor Author

I've added a expression so that the arrow size scales under different zoom levels. It'd be great to find the correct scaling factor though. This is how it looks right now 👀

arrow_scale_factor

@cjballard are you able to help here? I can guide you on how to set up the project and the dev environment (AS and such) so you could tweak the different factor values until we get something ✨ How does that sound?

@Cjballard-zz
Copy link

@Guardiola31337 Sure thing. Working with @danesfeder to get the environment set up & will start tweaking those.

@danesfeder danesfeder modified the milestones: 0.14.0, 0.15.0 May 30, 2018
@Cjballard-zz
Copy link

@Guardiola31337 Was doing some tinkering and I liked these sizes for each different stop...

stop(10, product(SHAFT_LINE_WIDTH_FACTOR, 2)),
stop(13, product(SHAFT_LINE_WIDTH_FACTOR, 4)),
stop(16, product(SHAFT_LINE_WIDTH_FACTOR, 6)),
stop(19, product(SHAFT_LINE_WIDTH_FACTOR, 8)),
stop(22, product(SHAFT_LINE_WIDTH_FACTOR, 10))

For each scaling factor for shaft width/casing and arrow size/casing.

Although I have a few design questions. One, it appears the casing for the arrow is more fuzzy and harder to achieve the same casing for the shaft layer. Is it because it’s image-based? Would like to have the same narrower casing as the shaft if possible. Here’s a screenshot:

1-arrow-casing

Also, I think it might be worth hiding the arrow when we reach a certain zoom level. Not sure which level this is specifically, but once the arrow becomes unhelpful and obstructive:

2-zoomlevels

So those would be my changes: match the casing for the shaft layer, hide it at a high zoom level and keep that size. I think it's looking great.

3-examples

@Guardiola31337
Copy link
Contributor Author

@cjballard

So those would be my changes: match the casing for the shaft layer, hide it at a high zoom level and keep that size.

Great feedback! Thanks a lot.

I've fixed the blurriness, added the hiding functionality under a certain zoom level (14) and tweaked some of the values to keep the sizes you suggested. This is what I came up with 👀

arrow_upcoming_maneuver

Would you be able to play around with the latest changes and let me know if we need to do any final adjustments?

@Guardiola31337 Guardiola31337 force-pushed the pg-604-upcoming-maneuver-arrow branch from c1d93c4 to e2e9211 Compare June 5, 2018 10:42
@Cjballard-zz
Copy link

@Guardiola31337 Yessir! I'll jump on that now.

@Guardiola31337 Guardiola31337 force-pushed the pg-604-upcoming-maneuver-arrow branch from e2e9211 to 682d878 Compare June 6, 2018 09:03
@Cjballard-zz
Copy link

@Guardiola31337 Looks good to me 👍 – that zoom level feels right to hide the arrow too.

@Guardiola31337
Copy link
Contributor Author

@cjballard

Looks good to me 👍 – that zoom level feels right to hide the arrow too.

Awesome! Thanks a lot for such a great feedback!

This is now ready for review 👀 @danesfeder @devotaaabel

@Guardiola31337 Guardiola31337 changed the title [WIP] Add upcoming maneuver arrow on the route line Add upcoming maneuver arrow on the route line Jun 6, 2018
@Guardiola31337 Guardiola31337 force-pushed the pg-604-upcoming-maneuver-arrow branch from 682d878 to d2ee32b Compare June 6, 2018 15:44
@Guardiola31337 Guardiola31337 added ✓ ready for review and removed ⚠️ DO NOT MERGE PR should not be merged! labels Jun 6, 2018
Copy link
Contributor

@danesfeder danesfeder left a comment

Choose a reason for hiding this comment

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

@Guardiola31337 thanks for all the work on this :shipit:

Noting [ ] Unit tests - with the PR that resolves #704, we should have a complete overhaul of this class that makes it easier to test.

@Guardiola31337 Guardiola31337 force-pushed the pg-604-upcoming-maneuver-arrow branch from d2ee32b to af50a6f Compare June 6, 2018 16:22
@Guardiola31337
Copy link
Contributor Author

Noting [ ] Unit tests - with the PR that resolves #704, we should have a complete overhaul of this class that makes it easier to test.

Yeah I agree!

@Guardiola31337 Guardiola31337 merged commit f26f7f9 into master Jun 6, 2018
@Guardiola31337 Guardiola31337 deleted the pg-604-upcoming-maneuver-arrow branch June 6, 2018 16:41
@danesfeder danesfeder mentioned this pull request Jun 21, 2018
11 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature request. platform parity Required to keep on par with iOS.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants