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 brewery quest #475

Merged
merged 17 commits into from
Nov 1, 2023
Merged

Add brewery quest #475

merged 17 commits into from
Nov 1, 2023

Conversation

mcliquid
Copy link

Fix for #472

"Nearby Values" Feature still missing from destination quest
And waiting for some clarity from the Community

"Nearby Values" Feature still missing from destination quest
@mcliquid
Copy link
Author

Okay, so far it's working!
https://github.com/Helium314/SCEE/assets/3351668/ccde889e-b756-48bf-b5b7-108eb8c99a21

@Helium314 However, I notice the same thing here as with the cuisine quest: When you start the quest, the far too large suggestion list covers the entire screen. Could the height be limited here or, alternatively, could a "Show suggestions" button be added?
https://github.com/Helium314/SCEE/assets/3351668/27e4f283-aeb8-45bd-99d6-b851340f3161

@Helium314
Copy link
Owner

I remember there is some way of limiting dropdown size, that could be used.
Showing the dropdown on start is something I find really useful, so that should at least still be optional.

The whole brewery form is almost a duplicate of the cuisine form, so it would make sense to have one base form to be used / adapted in both quests. I can do this.

The nearby part should be simple, but if you want I can add it myself.

@mcliquid
Copy link
Author

Would be awesome if you could take over. Will be much cleaner if you will do it 😉

@Helium314
Copy link
Owner

Would be awesome if you could take over. Will be much cleaner if you will do it 😉

Done. Though I didn't yet test the changes.

@mcliquid
Copy link
Author

Nice! I've tested it on my physical phone and jumped to Munich and it worked! The getHighlightedElements shows all kind of shops, restaurants and so on - not only beer relevant amenities - but that's fine. The Nearby-Value-Search also works good. I'm really happy with this one 👍

@mcliquid mcliquid marked this pull request as ready for review October 29, 2023 12:10
@mcliquid
Copy link
Author

From the app's point of view or from a technical point of view, would it be a disadvantage to extend the list of breweries even further? There are currently just under 100 entries in it, would it be bad if there were over 1000, for example?

@Helium314
Copy link
Owner

A longer list will slow down the suggestions, but I think even with 1000 entries the difference will not be noticeable.

The getHighlightedElements shows all kind of shops, restaurants and so on - not only beer relevant amenities - but that's fine

I guess that's ok. The highlighted elements are not useful for the quest anyway, but rather as helper when the user can't find the place.

Some more things:

  • The icon is in the debug folder (seems to be some weird Android Studio default when importing SVGs?)
  • The question sounds a little strange, and "Use suggested OSM tags" should be removed
    It will need to be changed anyway to clarify the draft / all beer question.
  • Maybe some answer can be added for the various value, if it makes any sense
  • Is there a way to tag places that have frequently changing beers? There are places that have a "constant" brewery, and some others that change weekly.

@mcliquid
Copy link
Author

  • The icon is in the debug folder (seems to be some weird Android Studio default when importing SVGs?)

This is strange - will have a look in the future if it's my fault or a strange setting. But fixed it.

  • The question sounds a little strange, and "Use suggested OSM tags" should be removed

I've took the same wording as the cuisine-Quest.

  • Maybe some answer can be added for the various value, if it makes any sense

Should it be part of the suggestion list or "Other Answer"-Option to make clear it's not a beer brand called "various"?

  • Is there a way to tag places that have frequently changing beers? There are places that have a "constant" brewery, and some others that change weekly.

In this case I would tag various. We have a craft beer shop/bar nearby they are advertising that they have over 600 different brands and always refrigerated!

@mcliquid
Copy link
Author

I found a proposal that might give us a little more clarification: https://wiki.openstreetmap.org/wiki/Proposal:Beer_details

It proposes that the key brewery should be used to list all brands that are served - regardless of whether they are tapped or bottled. In the proposal it is suggested to describe more details with drink:pilsner=draft or drink:pilsner=bottled and more detailed with drink:weizen:Paulaner=draft.

Accordingly, I would understand the quest as asking for all available brands. How which brand is served with exactly which variety should not be part of this quest.

Also, a few individuals on Talk:Key:brewery agree that the value various would not be helpful. Therefore, I would not promote this too much.

@Helium314
Copy link
Owner

various

If the quests really asks about all breweries (which it seems to do), then various should be an other answer, as it will definitely be necessary in many cases.

I've took the same wording as the cuisine-Quest.

The brewery tag may contain any brewery, even ones that are not in the suggestions.
The cuisine tag is less free, e.g. the user should not enter Burger, Burgers, burgers, hamburger, ... but use the common burger value.

@Helium314
Copy link
Owner

Looks like all is done now, right?
I'll wait for a few days before merging, just in case there are replies to your brewery post on the OSM forum.

@mcliquid
Copy link
Author

Now I've fixed the "Other Answer" issue - build is successful and behaviour works like planned. Let's wait some days, but I have the feeling that their is not really a opinion in the community.

@mcliquid
Copy link
Author

As a reference, this is the iD implementation of this tag. The short description is "Location sells beer from named brewery."
image

@Helium314
Copy link
Owner

Great, so the description and the title disagree with each other about draft vs all beers...

@mcliquid
Copy link
Author

I've tested it on multiple pois - works all good!
Maybe one thing: Should we remove any brewery-Tag when drink:beer=no is set from "No beer sold" Answer?

@Helium314
Copy link
Owner

I've tested it on multiple pois - works all good! Maybe one thing: Should we remove any brewery-Tag when drink:beer=no is set from "No beer sold" Answer?

I think brewery=no should remain. It's not contradicting drink:beer=no and some people may still use it and not be happy with removal.

@mcliquid
Copy link
Author

mcliquid commented Nov 1, 2023

Okay, then we leave it as it is

mcliquid and others added 2 commits November 1, 2023 11:23
filter: remove unnecessary parentheses, add resurvey
no beer: remove brewery tag unless it's brewery=no
ManyBeerAnswer -> ManyBeersAnswer
@Helium314
Copy link
Owner

I did some tiny changes and added resurvey after 6 years, which seemed reasonable to me

@Helium314 Helium314 merged commit 4389173 into Helium314:modified Nov 1, 2023
@mcliquid mcliquid deleted the brewery_quest branch November 1, 2023 11:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants