Skip to content

Conversation

@tomaszrybakiewicz
Copy link
Contributor

@tomaszrybakiewicz tomaszrybakiewicz commented Dec 2, 2022

Closes NAVAND-921

Description

  • Added new vector asset for the default navigation puck
    (LocationPuckOptions.Builder.navigationPuck(context: Context))
  • Added QA-Test-App “use legacy puck” toggle.

NOTE: Use of legacy puck can be restored as following:

navigationView.customizeViewStyles {
    locationPuckOptions = LocationPuckOptions.Builder(context)
        .defaultPuck(
            LocationPuck2D(
                bearingImage = ContextCompat.getDrawable(
                    context,
                    com.mapbox.navigation.ui.maps.R.drawable.mapbox_navigation_puck_icon,
                )
            )
        )
        .idlePuck(LocationPuckOptions.Builder.regularPuck(context))
        .build()
}

Screenshots or Gifs

QA-Test-App recording captured on Pixel 6

device-2022-12-02-113431.mp4

@tomaszrybakiewicz tomaszrybakiewicz added the UI Work related to visual components, Android Auto, Camera, 3D, voice, etc. label Dec 2, 2022
@tomaszrybakiewicz tomaszrybakiewicz requested a review from a team as a code owner December 2, 2022 16:35
@codecov
Copy link

codecov bot commented Dec 2, 2022

Codecov Report

Merging #6678 (93cd729) into main (67a81b4) will decrease coverage by 0.04%.
The diff coverage is 100.00%.

❗ Current head 93cd729 differs from pull request most recent head cfec898. Consider uploading reports for the commit cfec898 to get more accurate results

Impacted file tree graph

@@             Coverage Diff              @@
##               main    #6678      +/-   ##
============================================
- Coverage     72.54%   72.50%   -0.05%     
+ Complexity     5522     5510      -12     
============================================
  Files           772      772              
  Lines         29899    29891       -8     
  Branches       3533     3529       -4     
============================================
- Hits          21691    21673      -18     
- Misses         6793     6806      +13     
+ Partials       1415     1412       -3     
Impacted Files Coverage Δ
...box/navigation/ui/maps/puck/LocationPuckOptions.kt 97.11% <100.00%> (ø)
...ion/ui/maps/guidance/restarea/RestAreaProcessor.kt 60.71% <0.00%> (-15.65%) ⬇️
...x/navigation/ui/maps/building/BuildingProcessor.kt 68.00% <0.00%> (-4.23%) ⬇️
...navigation/core/reroute/MapboxRerouteController.kt 89.93% <0.00%> (-2.10%) ⬇️
...vigation/ui/maps/internal/ui/RouteLineComponent.kt 91.35% <0.00%> (-0.31%) ⬇️
...ava/com/mapbox/navigation/core/MapboxNavigation.kt 70.94% <0.00%> (-0.12%) ⬇️
.../navigation/core/trip/session/MapboxTripSession.kt 86.57% <0.00%> (-0.08%) ⬇️
.../main/java/com/mapbox/navigation/core/SetRoutes.kt 100.00% <0.00%> (ø)
...x/navigation/dropin/camera/CameraLayoutObserver.kt 94.11% <0.00%> (+0.24%) ⬆️

bearingImage = ContextCompat.getDrawable(
context,
R.drawable.mapbox_navigation_puck_icon,
R.drawable.mapbox_navigation_puck_icon2,
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we use a more appropriate name? Like mapbox_navigation_puck_icon_themed or sth like that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why? mapbox_navigation_puck_icon2 says it all. Version 2 of mapbox_navigation_puck_icon.

Copy link
Contributor

Choose a reason for hiding this comment

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

Because later we'll have mapbox_navigation_puck_icon3, mapbox_navigation_puck_icon4, mapbox_navigation_puck_icon10. How will we know which one stands for what? While "themed" describes it: the one that supports different themes.
But you've worked with UI and resources on the project much more that I did so no strong objections here, just a suggestion.

Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need the old drawable at all? Can we simply update mapbox_navigation_puck_icon instead of adding a new drawable?

Copy link
Contributor Author

@tomaszrybakiewicz tomaszrybakiewicz Dec 6, 2022

Choose a reason for hiding this comment

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

@dzinad We'll address this later if needed. mapbox_navigation_puck_icon2 is a good enough name for this asset.

Do we need the old drawable at all? Can we simply update mapbox_navigation_puck_icon instead of adding a new drawable?

@Zayankovsky We... no. Customers that already integrated Drop-In UI... yes. To allow those customers to switch back to the old puck easily, we should keep it. Besides, the asset is a vector graphic that weighs only 926 bytes.

@abhishek1508
Copy link
Contributor

LGTM. @stnikita1 Can you please take a look and let us know if this is fine before we merge it?

@stnikita1
Copy link

@abhishek1508 can we also update the generic puck please? And I reduced the size of the guidance puck
Generic Puck
Guidance Puck

@abhishek1508
Copy link
Contributor

@stnikita1 Thanks for adding the comment.
@tomaszrybakiewicz Let us use the resized navigation puck. Looks like it's a bit smaller than the previous design.

@stnikita1 We will address the round generic puck to have a better shadow in a follow up PR.

@tomaszrybakiewicz
Copy link
Contributor Author

tomaszrybakiewicz commented Dec 8, 2022

@stnikita1 Thanks for adding the comment. @tomaszrybakiewicz Let us use the resized navigation puck. Looks like it's a bit smaller than the previous design.

@stnikita1 We will address the round generic puck to have a better shadow in a follow up PR.

Done. I've updated the navigation puck to the one mentioned here #6678 (comment), and changed its fill colour to R.color.colorPrimary. Please see attached screen recording.

device-2022-12-08-133041.mp4

Unfortunately, Android doesn't support drop shadows in Vector Drawables. We'll have to figure something out and do this as a follow-up.

@tomaszrybakiewicz tomaszrybakiewicz force-pushed the NAVAND-921_tr-drop-in_new_navigation_puck branch 2 times, most recently from 34fcde4 to d62ce0e Compare December 8, 2022 19:59
@abhishek1508
Copy link
Contributor

@stnikita1 Can you take another look at it? The size of the puck has now been changed. I have shared the updated .apk on slack.

Copy link
Contributor

@abhishek1508 abhishek1508 left a comment

Choose a reason for hiding this comment

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

LGTM. We should wait for an approval from @stnikita1 before we merge

@tomaszrybakiewicz tomaszrybakiewicz force-pushed the NAVAND-921_tr-drop-in_new_navigation_puck branch from d62ce0e to f987d51 Compare December 9, 2022 02:11
@stnikita1
Copy link

Just reviewed the last apk and the puck looks surprisingly the same size. Could we try a smaller object?
Guidance Puck

@abhishek1508
Copy link
Contributor

Just reviewed the last apk and the puck looks surprisingly the same size. Could we try a smaller object? Guidance Puck

The resized .svg that you shared here had the following dimensions

android:width="61dp"
android:height="65dp"
android:viewportWidth="61"
android:viewportHeight="65"

The one you just shared in your last comment has the following dimensions

android:width="56dp"
android:height="59dp"
android:viewportWidth="56"
android:viewportHeight="59"

I just want to make sure that a value of 5px would make a difference.

cc @stnikita1

@stnikita1
Copy link

@abhishek1508 The previous iteration wasn't radical too but we didnt notice any difference. So, maybe this time it will work)

@tomaszrybakiewicz
Copy link
Contributor Author

@stnikita1 @abhishek1508 I've updated the navigation puck to the one attached here

Here's a comparison of all pucks

New Nav Puck A [default]
(56x59)
(stroke width 2)
New Nav Puck B
(61x65)
(stroke width 3)
Legacy Puck

@tomaszrybakiewicz tomaszrybakiewicz force-pushed the NAVAND-921_tr-drop-in_new_navigation_puck branch 2 times, most recently from 57adb8f to 2f2604e Compare December 12, 2022 19:48
@stnikita1
Copy link

stnikita1 commented Dec 13, 2022

@tomaszrybakiewicz thank you for the screens. I think we definitely should stay with the puck B (61x65, stroke width 3) It's better visible on the map. I fix the size, now the container is square.
Btw do we change the color in day/night mode as well?

Guidance Puck

@abhishek1508
Copy link
Contributor

Btw do we change the color in day/night mode as well?

Yes we do. These are the colors we use as our primary color for Drop-In UI in day and night mode:
Day - #373674
Night - #4D58B5

We will be using the color above for the puck to maintain consistency

cc @stnikita1

@tomaszrybakiewicz
Copy link
Contributor Author

@stnikita1 Updated. What do you think?

New Nav Puck
(64dp x 64dp)
(day)
New Nav Puck
(64dp x 64dp)
(day)
Legacy Puck
(75dp x 75dp)
(day)
Legacy Puck
(75dp x 75dp)
(night)

@stnikita1
Copy link

@tomaszrybakiewicz thanks for the screenshots! It looks great. But the shadow is gone for some reason. I double check the svg, the shadow should be there

@tomaszrybakiewicz
Copy link
Contributor Author

tomaszrybakiewicz commented Dec 14, 2022

@stnikita1 I've mentioned it in my previous comment. Android doesn't support drop shadows in Vector Drawables. We'll have to figure something out and do this as a follow-up.

Few alternatives:

  • use PNG asset instead
  • modify vector asset to make it more visible on white background (legacy puck had a translucent outline that helped)
  • use separate vector asset for the drop shadow (eg. below I've duplicated vector outline and applied legacy puck translucent outline color)
    Day Night

@tomaszrybakiewicz tomaszrybakiewicz force-pushed the NAVAND-921_tr-drop-in_new_navigation_puck branch from cf00fdf to d8896bd Compare December 15, 2022 16:17
@tomaszrybakiewicz
Copy link
Contributor Author

@stnikita1 Did some wizardry and applied a blur effect to the shadow asset using RenderScript.
Here's how it looks:

Day Night

@tomaszrybakiewicz tomaszrybakiewicz force-pushed the NAVAND-921_tr-drop-in_new_navigation_puck branch from d8896bd to af356e4 Compare December 15, 2022 17:50
@stnikita1
Copy link

@tomaszrybakiewicz awesome! Thank you!

Added QA-Test-App “use legacy puck” toggle.

Updated navigation puck vector drawable.

CHANGELOG entry

Updated navigation puck asset.
Added NavPuck spinner to QA-Test-App.

More changes to puck asset

Using RenderScript to apply blurred shadow to new navigation puck.
@tomaszrybakiewicz tomaszrybakiewicz force-pushed the NAVAND-921_tr-drop-in_new_navigation_puck branch from af356e4 to cfec898 Compare December 16, 2022 19:42
@tomaszrybakiewicz tomaszrybakiewicz enabled auto-merge (squash) December 16, 2022 19:43
@tomaszrybakiewicz tomaszrybakiewicz merged commit 5d52e38 into main Dec 16, 2022
@tomaszrybakiewicz tomaszrybakiewicz deleted the NAVAND-921_tr-drop-in_new_navigation_puck branch December 16, 2022 20:29
tomaszrybakiewicz added a commit that referenced this pull request Jan 10, 2023
Added QA-Test-App “use legacy puck” toggle.

Updated navigation puck vector drawable.

CHANGELOG entry

Updated navigation puck asset.
Added NavPuck spinner to QA-Test-App.

More changes to puck asset

Using RenderScript to apply blurred shadow to new navigation puck.
tomaszrybakiewicz added a commit that referenced this pull request Jan 12, 2023
Added QA-Test-App “use legacy puck” toggle.

Updated navigation puck vector drawable.

CHANGELOG entry

Updated navigation puck asset.
Added NavPuck spinner to QA-Test-App.

More changes to puck asset

Using RenderScript to apply blurred shadow to new navigation puck.
abhishek1508 pushed a commit that referenced this pull request Jan 12, 2023
Added QA-Test-App “use legacy puck” toggle.

Updated navigation puck vector drawable.

CHANGELOG entry

Updated navigation puck asset.
Added NavPuck spinner to QA-Test-App.

More changes to puck asset

Using RenderScript to apply blurred shadow to new navigation puck.
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.

5 participants