-
-
Notifications
You must be signed in to change notification settings - Fork 90
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
added campersites theme #213
Conversation
* campersite theme * minor corrections to documentation Note: I did not manage to test (no npm etc on this pc)
Please, also include the social-image as local asset: "https://upload.wikimedia.org/wikipedia/commons/9/93/Bar%C3%9Fel_Wohnmobilstellplatz.jpg" |
as requested #213 (comment)
Done |
"freeform": { | ||
"key": "power_supply", | ||
"addExtraTags": [ | ||
"power_supply=yes" |
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.
This is a bit weird. The freeform text field has a value "power_supply", but you will overwrite the value by the user by using power_supply=yes
. If you want to leave the freeform field, delete this extraTag, or use a different key e.g. "fixme=power_supply was filled in freeformly by a mapcomplete user, must be checked"
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 think I only wanted to show the "special values" here if they were filled in, not overrule them. I removed the addExtraTags
}, | ||
"condition": { | ||
"and": [ | ||
"name=" |
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.
By adding this condition, users will only see the question if the name is not yet known. The moment they fill in the name, they will not be able to edit it anymore - as this tagrendering will then dissappear. Is this intentional?
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.
Probably a misguided attempt when trying to not show "this place is called empty string" :)
Removed the condition
"en": "Does this place have a sanitary dump station?" | ||
}, | ||
"freeform": { | ||
"key": "", |
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.
Why define an empty key for freeform? This will cause bugs! Just remove the freeform block alltogether.
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.
done
}, | ||
{ | ||
"render": { | ||
"en": "internet access" |
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.
This is a weird rendering - it doesn't contain any substitutions... Maybe remove it, together with the freeform block? If you want to allow freeform text input, then at least use Internet is accessible via {internet_access}
or something, e.g. to make sense for values like wlan
, wifi
,...
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.
right, removing those and trying to render wlan and wifi as "yes" without it being an answer option
} | ||
} | ||
], | ||
"freeform": { |
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.
See comment above
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.
removing this
}, | ||
{ | ||
"render": { | ||
"en": "internet access price" |
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.
A rendering without freeform doesn't make sense - please remove 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.
removed
] | ||
} | ||
}, | ||
"reviews", |
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.
The mangrove-reviews
-block can take up a big chunk of space. While not wrong to place it here, I strongly recommend to move it to the bottom of the tagrenderings, as the tagrenderings below will probably be missed by the users
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.
makes total sense, I've put it in the back
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.
A good start, but there are some things that should be improved first before merging
"reviews", | ||
{ | ||
"render": { | ||
"en": "toilets" |
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.
Same issue as above: either allow a freeform text field and use the value, or remove them
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.
done
"en": "More details about this place: {description}" | ||
}, | ||
"question": { | ||
"en": "Would you like to add a general description of this place? (please keep it objective; opinions go into the reviews)" |
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 have noticed that people tend to repeat information that was already stated in the earlier asked questions; it doesn't hurt to state something like "Do not repeat information previously asked or shown above"
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.
done
{ | ||
"if": { | ||
"and": [ | ||
"fee=no" |
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 you copy-paste from the tagrenderings here? You are marking icon sizes here...
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 idea how this happened
] | ||
}, | ||
"then": { | ||
"en": "for free" |
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.
"for free" is not a valid color as well
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.
removed (same as above)
"en": "camper site" | ||
}, | ||
"description": { | ||
"en": "Add a new official camper site. Please zoom in sufficiently for exact placement. Don't copy info from copyrighted content. Your data will be added straight to the global OpenStreetMap database!" |
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.
These warnings are already included in the popup itself - they shouldn't be repeated. If you feel the current popup is not clear enough, open an issue or PR to fix that.
The description is meant to give a clarification on exactly what a camper site is, eventually with pictures.
FOr example, I added this on cyclofix as in some (non-english-speaking) countries, a "bicycle repair station" was confused with a "bicycle repair shop". It looks like this:
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.
removed the unneeded parts and extended the description with more about the actual POI type
}, | ||
{ | ||
"render": { | ||
"en": "chemical toilet" |
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.
As usual - I know the custom theme generator is buggy...
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.
done
}, | ||
{ | ||
"render": { | ||
"en": "access tag" |
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.
Once more
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.
done
"then": { | ||
"en": "Anyone can use this dump station" | ||
}, | ||
"hideInAnswer": false |
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.
False is the default - you can omit this; but it is not wrong per se.
BTW: isn't access=yes
the preferred tag over access=public
? Then maybe this one should be hidden and the one below be shown.
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.
oh dear. Of course. Fixed.
}, | ||
{ | ||
"render": { | ||
"en": "This place is operated by {operator}" |
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.
The common questions for all layers can be moved to roamingRenderings
of the the theme - these are tagRenderings that are added to every object in every layer, this avoids repetition and makes fixes and translations easier.
(If you want to hide a tagRendering for a single layer, this can be done by the condition on the tagrendering)
"en": "sanitary dump station" | ||
}, | ||
"description": { | ||
"en": "Add a new sanitary dump station. Please zoom in sufficiently for exact placement. Don't copy info from copyrighted content. Your data will be added straight to the global OpenStreetMap database!" |
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.
Same remark as the other preset: the general warning message should be handled by MapComplete, not every theme individually
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.
done
I have merged, yet noticed a few issues:
Furthermore, I've added white circles behind the icons, to make them stand out more and made the 'sanitary-dump-station'-rendering a bit smaller. See 7e61b08 for my changes. |
I've also made some tweaks to the codebase in order to have the roaming tagrenderings appear just before the questions |
Note: I did not manage to test (no npm etc on this pc)