-
-
Notifications
You must be signed in to change notification settings - Fork 861
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
[generator] Add miniature_golf type #7713
Conversation
not sure what the failed smoke test means :( something to do with it being three levels? |
@vng can you please take a look at the failing test? https://github.com/organicmaps/organicmaps/actions/runs/8469901850/job/23206321535#step:9:44459 |
fixed type, strings, now using golf icon. still need to do a map regen, but should be good now |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
data/categories.txt
Outdated
@@ -7957,6 +7957,48 @@ sw:Uwanja wa gofu|michezo | |||
fa:زمین گلف | |||
mr:गोल्फचे मैदान | |||
|
|||
leisure-miniature_golf | |||
en:Mini-Golf|Minigolf|Miniature Golf|Putt Putt |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Mini-Golf" can produce a mess in current categories matching implementation.
Remove this entry or combine it with leisure-golf_course.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So the issue is in the hyphen like in #7774 (comment), right?
Here we can use a "Mini Golf" instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, Mini Golf - is the same. Category matching works for each token (now). Take it into account.
"Mini" will be confusing.
"Putt" also will match it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
discussion: #7709 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So after the discussion my understanding is that while the "mini" part will certainly add some unrelated results (e.g. "mini-market") the full "mini golf" matches will be in the top most relevant results, so usability will be OK.
IMO it would be worse if typing "mini golf" (as its a very common term) will mix actual mini golf courses somewhere down inbetween unrelated stuff.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At your decision, but IMO en:4Minigolf|Miniature Golf|Putt Putt
will be enough here with suggest.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vng I'm guessing there should be no spaces or dashes for every language, not just English, right?
Current translations (some have a dash, some have a space):
en = Minigolf
af = Minigolf
ar = ميني غولف صغير
az = Minigolf
be = Міні-гольф
bg = Миниголф
ca = Minigolf
cs = Minigolf
da = Minigolf
de = Minigolf
el = Μίνι γκολφ
es = Mini golf
et = Minigolf
eu = Minigolf
fa = مینی گلف
fi = Minigolf
fr = Mini golf
he = מיני גולף
hi = मिनी गोल्फ
hu = Minigolf
id = golf mini
it = Mini golf
ja = ミニゴルフ
ko = 미니골프
lt = Minigolfas
mr = मिनीगोल्फ
nb = Minigolf
nl = Minigolf
pl = Mini golf
pt = Minigolfe
ro = Minigolf
ru = Минигольф
sk = Minigolf
sv = Minigolf
sw = Minigofu
th = มินิกอล์ฟ
tr = Mini golf
uk = Міні-гольф
vi = Golf mini
zh-Hans = 迷你高尔夫
zh-Hant = 迷你高爾夫
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, take this and will merge:
en:Minigolf
af:Minigolf
az:Minigolf
be:Мінігольф
bg:Миниголф
ca:Minigolf
cs:Minigolf
da:Minigolf
de:Minigolf
es:Minigolf
et:Minigolf
eu:Minigolf
fi:Minigolf
fr:Minigolf
hu:Minigolf
it:Minigolf
ja:ミニゴルフ
ko:미니골프
mr:मिनीगोल्फ
nb:Minigolf
nl:Minigolf
pl:Minigolf
pt:Minigolfe
ro:Minigolf
ru:Минигольф
sk:Minigolf
sv:Minigolf
sw:Minigofu
th:มินิกอล์ฟ
tr:Minigolf
uk:Мінігольф
zh-Hans:迷你高尔夫
zh-Hant:迷你高爾夫
fixed 👍️ @vng merge? |
@RedAuburn looks like you forgot regen commits :) |
argh oops 😅 ty for reminder |
Please, remove af, az languages from categories. |
hmm 🤔 will remove 👍️ |
Signed-off-by: Harry Bond <hrbond@pm.me>
Signed-off-by: Harry Bond <me@hbond.xyz>
Signed-off-by: Harry Bond <me@hbond.xyz>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did the radar icon change in size?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
huhh.. not sure why, does it look different in-app?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not in the app. I just saw it from the automatic generated differences between icon folders. Don't worry, it shouldn't be the case. Cheers.
Adopts styles from leisure=golf_course
map regeneration needed (will do one later today to test)
fixes #4587