[ios] Remove invisible taxi button from route preview #3456

RedAuburn · 2022-09-19T16:54:39Z

gnojus commented

2022-09-19 16:54:39 +00:00

(Migrated from github.com)

As I understand, taxi routing was remove a while ago, but invisible taxi icon remained in route preview. Deleted it; icons are now nicely center aligned.

Was

After this fix

As I understand, taxi routing was remove a while ago, but invisible taxi icon remained in route preview. Deleted it; icons are now nicely center aligned. **Was** ![IMG_0580](https://user-images.githubusercontent.com/46261165/191071350-96852087-3027-46c0-a245-b937fc81c306.jpg) **After this fix** ![IMG_0581](https://user-images.githubusercontent.com/46261165/191071353-b49913fe-dcb4-4581-82df-8513b855fbad.jpg)

👍 1

biodranik commented

2022-09-19 20:09:33 +00:00

(Migrated from github.com)

Thanks! Can you please replace the taxi icon with a helicopter icon (and leave it hidden for now)? We plan to add a "helicopter" routing :)

gnojus commented

2022-09-22 11:25:05 +00:00

(Migrated from github.com)

Took a while to confirm the helicopter wasn't a joke:)

Added back the button to storyboard and unmarked "Installed" to hide it. It does not have an icon and is not connected to anything else.

I did not "Uninstall" the constraints though. This gives out a few warnings, but it will require less clicks to re-enable the helicopter button. Is that OK?

Took a while to confirm the helicopter wasn't a joke:) Added back the button to storyboard and unmarked "Installed" to hide it. It does not have an icon and is not connected to anything else. I did not "Uninstall" the constraints though. This gives out a few warnings, but it will require less clicks to re-enable the helicopter button. Is that OK?

biodranik (Migrated from github.com) reviewed 2022-09-24 17:01:43 +00:00

biodranik (Migrated from github.com) left a comment

Thanks! What kind of warnings did you get?

Can you please check how this icon will look like (and share a screenshot)? https://github.com/mapbox/maki/blob/main/icons/heliport.svg

If it's ok, you can add it (and probably update our data/copyright.html with Maki license).

Thanks! What kind of warnings did you get? Can you please check how this icon will look like (and share a screenshot)? https://github.com/mapbox/maki/blob/main/icons/heliport.svg If it's ok, you can add it (and probably update our data/copyright.html with Maki license).

gnojus commented

2022-09-26 15:31:56 +00:00

(Migrated from github.com)

Below are the image of how the helicopter looks. Also while there, I tried out the bicycle icon from the same repo. IMO it looks better

in comparison with the old bicycle icon:

Below are the image of how the helicopter looks. Also while there, I tried out the bicycle icon from the same repo. IMO it looks better ![IMG_0609](https://user-images.githubusercontent.com/46261165/192317662-fb25277a-b3ff-4fd7-bae1-dcb22470f62f.jpg) in comparison with the old bicycle icon: ![IMG_0581](https://user-images.githubusercontent.com/46261165/192318217-01f4a1ff-e65f-406e-8d50-bc3aa598183e.jpg)

👍 1

gnojus commented

2022-09-26 16:51:12 +00:00

(Migrated from github.com)

The warnings given are multiple of Constraint referencing items turned off in current configuration. Turn off this constraint in the current configuration. These can be turned of just by uninstalling the constraints too (not just the button).

The warnings given are multiple of `Constraint referencing items turned off in current configuration. Turn off this constraint in the current configuration`. These can be turned of just by uninstalling the constraints too (not just the button).

RedAuburn commented

2022-09-26 16:56:19 +00:00

i like the new bicycle icon, i'll make a PR adding these things to android as well if this is approved 👍

biodranik commented

2022-09-27 07:14:41 +00:00

(Migrated from github.com)

Thanks! The icons look good. Maybe the maki icons size can be slightly increased somehow, to (almost) match other icons height? The same "resized" icon can be reused on all platforms.

Also please don't forget about the copyright.

Thanks! The icons look good. Maybe the maki icons size can be slightly increased somehow, to (almost) match other icons height? The same "resized" icon can be reused on all platforms. Also please don't forget about the copyright.

biodranik commented

2022-09-27 07:15:07 +00:00

(Migrated from github.com)

@nojus297 to confirm: in this PR helicopter is invisible, right?

gnojus commented

2022-09-27 09:20:50 +00:00

(Migrated from github.com)

@nojus297 to confirm: in this PR helicopter is invisible, right?

yes, invisible and not taking space (allowing other icons to be centered).

> @nojus297 to confirm: in this PR helicopter is invisible, right? yes, invisible and not taking space (allowing other icons to be centered).

gnojus commented

2022-09-27 09:25:49 +00:00

(Migrated from github.com)

Thanks! The icons look good. Maybe the maki icons size can be slightly increased somehow, to (almost) match other icons height? The same "resized" icon can be reused on all platforms.

I scaled both new icons by 220% (with inkscape). Do they need to be even bigger? (Note that the new bike is bigger than the old one, and both of the new icons are closer to the edges than ones of car and pedestrian).

Also the new icons are more horizontal, so scaling them to match height might make them very wide.

> Thanks! The icons look good. Maybe the maki icons size can be slightly increased somehow, to (almost) match other icons height? The same "resized" icon can be reused on all platforms. I scaled both new icons by 220% (with inkscape). Do they need to be even bigger? (Note that the new bike is bigger than the old one, and both of the new icons are closer to the edges than ones of car and pedestrian). Also the new icons are more horizontal, so scaling them to match height might make them very wide.

biodranik commented

2022-09-27 11:57:32 +00:00

(Migrated from github.com)

The idea is to make them slightly more recognizable to people with bad eyesight. Let's compare the look of the current version with a slight increase.

gnojus commented

2022-09-27 18:01:00 +00:00

(Migrated from github.com)

Sharing some screenshots to compare:
previous (220% from maki)

scaled by 5% (231% total)

scaled by 10% (241% total)

Sharing some screenshots to compare: previous (220% from maki) ![IMG_0611](https://user-images.githubusercontent.com/46261165/192601939-fd285906-a7d9-4385-a48e-aeb4703fab02.PNG) scaled by 5% (231% total) ![IMG_0613](https://user-images.githubusercontent.com/46261165/192601945-33d098b7-67e4-4131-a997-ff3fa10565d0.PNG) scaled by 10% (241% total) ![IMG_0614](https://user-images.githubusercontent.com/46261165/192601950-39dbf411-37c1-4f5e-8fd4-36743bba287a.PNG)

biodranik commented

2022-09-29 22:32:29 +00:00

(Migrated from github.com)

The difference is almost negligible, but bigger looks better. What do you think?

gnojus commented

2022-09-30 12:20:01 +00:00

(Migrated from github.com)

I prefer +5%, I'm okay with +10% but definitely would not want to go any bigger, already looks a bit cramped to my eyes.

However, if we go with +10%, we should probably scale the car as well, as it looks a bit small in comparison (when not selected)

I prefer +5%, I'm okay with +10% but definitely would not want to go any bigger, already looks a bit cramped to my eyes. However, if we go with +10%, we should probably scale the car as well, as it looks a bit small in comparison (when not selected) ![image](https://user-images.githubusercontent.com/46261165/193268085-c7c72d9b-51fc-4a5a-b388-62ced1d4971b.png)

biodranik commented

2022-09-30 20:41:02 +00:00

(Migrated from github.com)

Good point. Ok, 5% is also good. Let's move on with implementing the helicopter routing in the code in another PR :) @nojus297 can you start to draft it? We will help by adding support in the core.

👍 1

gnojus commented

2022-10-04 00:03:37 +00:00

(Migrated from github.com)

Added the new icons with the license. Also some Helicopter enum values, guessing best switch case in some places. (Helicopter is currently not visible).

biodranik (Migrated from github.com) reviewed 2022-10-04 00:53:57 +00:00

biodranik (Migrated from github.com) left a comment

Good start! Let's check how other routers are working and try to do the same. The difference will be is that the route can be calculated instantly via specified points.

iphone/Maps/Classes/CustomViews/NavigationDashboard/Views/RoutePreview/MWMRoutePreview.mm

					
				@ -104,9 +104,6 @@ static CGFloat const kDrivingOptionsHeight = 48;

				{

				  for (auto const & progress : m_progresses)

				    progress.second.state = MWMCircularProgressStateNormal;

biodranik (Migrated from github.com) commented

2022-10-04 00:45:40 +00:00

These comments can be removed.

iphone/Maps/Core/Location/MWMLocationManager.mm

biodranik (Migrated from github.com) commented

2022-10-04 00:50:54 +00:00

No, it's better to insert it at the end. Please try to understand what the previous code did and decide if it is relevant here.

biodranik (Migrated from github.com) commented

2022-10-04 00:51:18 +00:00

Also, sometimes it is better to leave a warning and come back to fix it later.

gnojus (Migrated from github.com) reviewed 2022-10-05 08:07:05 +00:00

iphone/Maps/Core/Location/MWMLocationManager.mm

gnojus (Migrated from github.com) commented

2022-10-05 08:07:05 +00:00

Is this ok now?

gnojus commented

2022-10-05 08:11:34 +00:00

(Migrated from github.com)

Are we planning to implement the actual routing in this PR too? Could we merge this PR now and add routing in separate PR, leaving this one just for icons?

biodranik commented

2022-10-06 21:24:07 +00:00

(Migrated from github.com)

It is a good idea. But then let's create a new PR and move there these enums and switch cases. There should be no any side effect in the master.

gnojus commented

2022-10-07 19:13:46 +00:00

(Migrated from github.com)

Removed all the helicopters from routing logic.

biodranik (Migrated from github.com) approved these changes 2022-10-07 21:47:03 +00:00

biodranik (Migrated from github.com) left a comment

Thanks! Can be squashed if all checks pass.

gnojus commented