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

[ios][android] Add fee indicator to Place Page and search results #5883

Merged
merged 12 commits into from
Nov 21, 2023

Conversation

dvdmrtnz
Copy link
Contributor

@dvdmrtnz dvdmrtnz commented Aug 31, 2023

This PR adds a fee indicator to the Place Page in iOS. This icon will show in any place that has a fee=yes property.

@matheusgomesms
Copy link
Contributor

Great job!!! I always wanted that differentiation! Regarding the icon, I would personally use the dollar sign. I think it’s more recognizable than the bag one.

map/place_page_info.cpp Outdated Show resolved Hide resolved
@dvdmrtnz
Copy link
Contributor Author

dvdmrtnz commented Sep 1, 2023

Great job!!! I always wanted that differentiation! Regarding the icon, I would personally use the dollar sign. I think it’s more recognizable than the bag one.

🤔 I’m thinking that maybe it could be better to have it in text: Fee/No fee. That way we could have info about the feature being free, as opposed to no info available. Because I don’t thing there is a suitable "No fee" icon. WDYT?

@dvdmrtnz
Copy link
Contributor Author

dvdmrtnz commented Sep 1, 2023

I've changed it to a localized Paid/Free text.

@RedAuburn
Copy link
Sponsor Member

I've changed it to a localized Paid/Free text.

this looks good :)

@Jean-BaptisteC
Copy link
Member

What about Android?

@dvdmrtnz
Copy link
Contributor Author

dvdmrtnz commented Sep 1, 2023

What about Android?

I’m not 100% sure but I think it should actually work in Android with these changes, since I’ve made this modification in the c++ core

@Jean-BaptisteC
Copy link
Member

I see nothing on Android with latest build

@dvdmrtnz
Copy link
Contributor Author

dvdmrtnz commented Sep 1, 2023

I see nothing on Android with latest build

Maps need to be regenerated because there is a new metadata property

Copy link
Member

@map-per map-per left a comment

Choose a reason for hiding this comment

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

Added a suggestion for the german translation

data/strings/strings.txt Outdated Show resolved Hide resolved
@dvdmrtnz
Copy link
Contributor Author

dvdmrtnz commented Sep 1, 2023

I see nothing on Android with latest build

I just tested it on Android. A minor change was needed, and now it works there too.

@dvdmrtnz dvdmrtnz changed the title [ios] Add fee indicator to Place Page [ios][android] Add fee indicator to Place Page Sep 1, 2023
@biodranik
Copy link
Member

Did you see #1312 and #2254 ?

How important is it to be able to quickly search by the value of the fee tag?

@dvdmrtnz
Copy link
Contributor Author

dvdmrtnz commented Sep 2, 2023

Did you see #1312 and #2254 ?

How important is it to be able to quickly search by the value of the fee tag?

Yes, I saw those threads. I guess that you are asking me this because I took the "metadata" approach to adding this info, instead of adding it as a "type", right? I thought it was easier to add fee=* as metadata property and more scalable (given the limit of 7 types per feature), specially if we want to add some more properties like takeaway=*, delivery=*, capacity=*, drive_through=* and similar properties of other bigger places.

@biodranik
Copy link
Member

fee is important for some other types too like parkings, toilets, etc. Using metadata in the search is possible, but it will be very slow.

Copy link
Member

@vng vng left a comment

Choose a reason for hiding this comment

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

UI - no problem here.

I'm planning to refactor fee, access, etc #1312

Storing fee in metadata is not a good idea. At least because of fast reading for rendering. I'm sure, we will render some dynamic icons someday depending on these tags.

@dvdmrtnz
Copy link
Contributor Author

dvdmrtnz commented Sep 2, 2023

UI - no problem here.

I'm planning to refactor fee, access, etc #1312

Storing fee in metadata is not a good idea. At least because of fast reading for rendering. I'm sure, we will render some dynamic icons someday depending on these tags.

Okay. I guess we'll have to wait until the fee refactoring then.

@pastk
Copy link
Contributor

pastk commented Sep 6, 2023

Storing fee in metadata is not a good idea. At least because of fast reading for rendering. I'm sure, we will render some dynamic icons someday depending on these tags.

I think this solution is fine for the interim.

The fee/access refactoring mentioned is a very long shot. And dynamic icons is even longer one :) No need to wait for them when we can improve UX now.

Code changes in this PR are small, so no problem to re-do when we have a new system in place.

@pastk
Copy link
Contributor

pastk commented Sep 11, 2023

We've been discussing it and decided that adding separate fee-yes/fee-no types seems to be a better long-term approach.

@dvdmrtnz
Copy link
Contributor Author

We've been discussing it and decided that adding separate fee-yes/fee-no types seems to be a better long-term approach.

I have tried adding to mapcss-mapping.csv:

fee|yes;[fee=yes];;name;int_name;1583;
fee|no;[fee=no];;name;int_name;1584;

But it doesn't seem to be working 🤔. I have regenerated maps and everything but it doesn't show up in the raw types. Any idea why?

https://www.openstreetmap.org/node/9341902098#map=19/42.81552/-1.64178

@pastk
Copy link
Contributor

pastk commented Oct 27, 2023

But it doesn't seem to be working 🤔. I have regenerated maps and everything but it doesn't show up in the raw types. Any idea why?

Types that don't have any styles defined are discarded by the generator.
So you've got to add these new types into "exceptions" alongside cuisine and wheelchair in indexer/feature_visibility.cpp::IsUsefulNondrawableType().

Please add a note about this behavior into the comment in the mapcss-mapping.csv.

Also note that its better to use fee|yes;[fee?];... as it'll include non-yes values like fee=10 (basically everything except fee=no).

@dvdmrtnz
Copy link
Contributor Author

dvdmrtnz commented Oct 28, 2023

@pastk @vng @biodranik PTAL. Now using fee-yes, fee-no types, instead of metadata

@dvdmrtnz
Copy link
Contributor Author

Seems to me like a double-negative but Iet's see if any of those will get displayed properly...

0024+043F+20E0: $͏⃠ 0024+FEFF+20E0: $⃠ 0024+2060+20E0: $⁠⃠ 1F4B2+2060+20E0: 💲⁠⃠ 1F10F: 🄏

I'd go for option 5. or 7. too, though.

@pastk
Copy link
Contributor

pastk commented Nov 11, 2023

IMHO none of those look good enough... Also their display might be dependent on available fonts, isn't it?

It might look OK if we make our own clean looking icon with a crossed dollar sign. But at the expense of hardcoding its display. And I don't think its worth it, a textual "Free" is a better option IMO.

@dvdmrtnz
Copy link
Contributor Author

Some more options:

\u0024\u20dd
$⃝

\u0024\u20e3
$⃣

\u0024\u20e5
$⃥

\u0024\u20ea
$⃪

\u0024\u20eb
$⃫

\u0024\u0336

\u0024\u20dd\u20e5
$⃝⃥

\u0024\u20dd\u0336
$⃝̶

\u0024\u20dd\u20eb
$⃝⃫

It's quite funny what you can do with unicode and diacritics 🤣. Here's the list of unicode diacritics: https://en.wikipedia.org/wiki/Diacritic#List_of_diacritics_in_Unicode

By the way I am using this website to try out the characters: https://www.branah.com/unicode-converter

Some of these look okay in iOS but then look quite bad on Android 😂

@dvdmrtnz
Copy link
Contributor Author

dvdmrtnz commented Nov 12, 2023

I'm setting it as Toilet • Free / Toilet • 💲 / Toilet

@biodranik
Copy link
Member

@vng PTAL

Copy link
Member

@vng vng left a comment

Choose a reason for hiding this comment

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

LGTM

generator/generator_tests/osm_type_test.cpp Outdated Show resolved Hide resolved
generator/generator_tests/osm_type_test.cpp Outdated Show resolved Hide resolved
data/mapcss-mapping.csv Outdated Show resolved Hide resolved
search/intermediate_result.cpp Outdated Show resolved Hide resolved
search/intermediate_result.cpp Outdated Show resolved Hide resolved
Signed-off-by: David Martinez <47610359+dvdmrtnz@users.noreply.github.com>
Signed-off-by: David Martinez <47610359+dvdmrtnz@users.noreply.github.com>
Signed-off-by: David Martinez <47610359+dvdmrtnz@users.noreply.github.com>
Signed-off-by: David Martinez <47610359+dvdmrtnz@users.noreply.github.com>
Signed-off-by: David Martinez <47610359+dvdmrtnz@users.noreply.github.com>
Signed-off-by: David Martinez <47610359+dvdmrtnz@users.noreply.github.com>
Signed-off-by: David Martinez <47610359+dvdmrtnz@users.noreply.github.com>
Signed-off-by: David Martinez <47610359+dvdmrtnz@users.noreply.github.com>
Signed-off-by: David Martinez <47610359+dvdmrtnz@users.noreply.github.com>
Signed-off-by: David Martinez <47610359+dvdmrtnz@users.noreply.github.com>
Signed-off-by: David Martinez <47610359+dvdmrtnz@users.noreply.github.com>
Signed-off-by: David Martinez <47610359+dvdmrtnz@users.noreply.github.com>
@vng vng merged commit 8d0dffe into organicmaps:master Nov 21, 2023
14 checks passed
@dvdmrtnz dvdmrtnz deleted the fee branch November 21, 2023 21:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Android Android development Enhancement New feature or request, an improvement of some existing feature iOS iOS development POI Info Feature metadata, OSM tags that are displayed in Place Page UI User interface issues
Projects
None yet