-
Notifications
You must be signed in to change notification settings - Fork 21
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
updated icons and more shields #262
Conversation
bubble-wrap-style.yaml
Outdated
filter: function() { return (feature.shield_text.length === 5) } | ||
sprite: | | ||
function() { | ||
var text_len = feature.shield_text.length ? feature.ref.length; |
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 an incorrect ternary syntax:
feature.shield_text.length ? feature.ref.length;
you need a :
with the alternate value, or I think what you actually want to do is maybe this?
feature.shield_text.length || feature.ref.length
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 causing the current v9 Bubble Wrap (released from this un-merged branch? I can't tell) to fail in various region/zoom combinations.
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.
fixed in 3374392
bubble-wrap-style.yaml
Outdated
sprite: | | ||
function() { | ||
var text_len = feature.shield_text.length ? feature.ref.length; | ||
return ( 'FR:D-' + feature.text_len + 'char'); |
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.
Related to above, not sure this is what you actually mean here? Is there a text_len
property on the feature itself? I assume you want to use the variable you just defined above, like:
return ( 'FR:D-' + text_len + 'char');
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.
fixed in 3374392
bubble-wrap-style.yaml
Outdated
international-shields: | ||
filter: | | ||
function() { | ||
return feature.shield_text && |
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.
You need to make the precedence clearer here, cases where feature.shield_text
is null are getting through. Should be:
return feature.shield_text && (
/^AM:AM.*$/.test(feature.network) ||
...
/^ZA:R.*$/.test(feature.network)
)
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.
fixed in 3374392
With my most recent commits, I'm not finding any errors right now when trying a broad range of locations (that doesn't mean they aren't there though!). Some of the same fixes need to be applied to Refill (and probably others?). |
bubble-wrap-style.yaml
Outdated
((feature.ref && feature.network == 'e-road') && | ||
/^A .*$/.test(feature.ref) || | ||
/^N .*$/.test(feature.ref) ) | ||
return ((feature.ref && !feature.shield_text) && (/^A .*$/.test(feature.ref) || /^N .*$/.test(feature.ref))) || |
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'm honestly not entirely sure what we're trying to test for here, so the syntax now works (I think the old one was short-circuiting some logic), but it may not be correct...
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 for A-roads and N-roads in Europe (like in France).
Merging this now to reflect what's been released on Nextzen as v9 which is based on beb020b. Since then @bcamper has made a couple bug fixes, so after merging we should release a |
No description provided.