-
-
Notifications
You must be signed in to change notification settings - Fork 13
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 Quest: building:material #569
base: modified
Are you sure you want to change the base?
Add Quest: building:material #569
Conversation
Metal, steel and tin is a bit confusing. But yeah, all are in use... |
Yes, I said to myself that I couldn't select a value. Maybe change the message to indicate that only experts can use it? Or other photos but I myself am not an expert on the subject. |
Perhaps drop |
What do you say to these tags? I like having the choice, especially when I know the subject well. In this case, it would mean that contributors are being forced to use metal, which may or may not be the case. |
As a comparison to |
@mcliquid thanks for shedding some light on that -- I have to acknowledge I was not clear on the distinction of some of these. @ravenfeld the rest looks good to me! the images are also very illustrative. |
@mcliquid |
@HolgerJeromin I definitely agree with the target group of StreetComplete. But here we are talking about SCEE - the StreetComplete Expert Edition, right? |
I did not have a look at basically anything yet, but please massive reduce image size. This PR alone is adding ~4 megabytes to app size. Also not sure about the general style when I look at the images. Some of the tags appear to be categories, e.g. metal -> tin+steel, or stone -> sandstone. Btw what is |
@mcliquid @Claurt07 @Un1matr1x |
TL;DR: perhaps go just with
Actually, it is copper salts that produce that green patina, not tin. Tin is major component of solder, and tin foil is what we used to use to make tin foil hats to protect us from government (or alien) mind-reading devices (or to wrap food) 😃 - although it has mostly been superseded by aluminium foil nowdays, but name stuck. As for building material, while I generally prefer detailed answers to be available in SCEE, I'm not so sure here. Basically, only copper is distinctive, other metals not so much. So I'd be fine with just Most people (who are not material scientists or are unwilling to scratch the surfaces to determine what it is) will be hard pressed to tell the difference between corrugated galvanized iron (sometimes called "wriggly tin" to add to the confusion, even if Zinc is used for galvanization), steel that has been coated with Zinc-Aluminium, iron/steel coated with thin layer of tin (also known as "Tinplate"), various aluminium alloys (aluminium+manganese, aluminium+magnesium etc.) and steel coated with alloy of lead and tin (also called Tinplate colloquially, but more precisely Terne, even if in original coating there was order of magnitude more lead then tin; but the coating is not load bearing (majority) material anyway). The use of actual |
I agree, let's start simple and pick only the significant values. You can always extend a quest later! |
What do you want me to display then? |
Drop |
As far as I can see, those are several issues mentioned so far that prevent this PR:
|
app/src/main/java/de/westnordost/streetcomplete/quests/building_material/AddBuildingMaterial.kt
Outdated
Show resolved
Hide resolved
2302c71
to
d6e0119
Compare
f033369
to
660aee5
Compare
Hello, I'm coming back to my PR. I've used my version a lot, which I've modified, and it's not very good for the community. I'm going to make one PR by one PR to avoid having too many open with you. I've used ImageOptim (https://imageoptim.com/versions.html |
Thanks, that's a considerable reduction in size! |
<string name="quest_buildingMaterial_title">"What is the overall surface material of building walls here?"</string> | ||
<string name="quest_buildingPartMaterial_title">"What overall material is this building part made of?"</string> |
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.
Shouldn't the building part title also refer to surface material?
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.
I didn't understand the remark.
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.
quest_buildingMaterial_title
asks for overall surface material, while quest_buildingPartMaterial_title
asks for overall material.
Is this difference intentional, or an oversight?
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, it's just that I'm bad at English.
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.
I modified it but I'm still not good at English.
This looks mostly fine now, only maybe a few details left, and @Claurt07's question is still open:
|
@ravenfeld this SCEE PR (and few others by you) are almost finished and need just a little attention from you to make them mergable. Would you perhaps have time to do that in nearby future? ❤️ |
I'm going to try it this weekend but with the application I'm developing for Panorama + work, I'd moved on to something other than SCEE. Sorry for the inconvenience. |
No worries @ravenfeld, life priorities change; it is understood. Thank you for your contributions so far, and any that may happen on the weekend should you manage it! ❤️ If situation changes, we'll love to have to you contribute again; but in the meantime, given the explanation, I'll assume it is fine with you for someone else interested to pick up those half-finished PRs (after the weekend) and bring them to completion? |
It's not my property. Anyone can take over the PR without any worries |
660aee5
to
1b6e311
Compare
I think this is fine now, thank you @ravenfeld. |
I think it's mostly fine, only minor comment
|
I'm going to split the request in 2.
#443
Creating a quest to add building material