Skip to content
This repository has been archived by the owner on Aug 14, 2024. It is now read-only.

Commit

Permalink
add comments to reduce confusion risks
Browse files Browse the repository at this point in the history
  • Loading branch information
@matkoniecz @westnordost
matkoniecz authored and westnordost committed Jun 1, 2020
1 parent 3ca203b commit 5dcfd32
Showing 1 changed file with 3 additions and 0 deletions.
3 changes: 3 additions & 0 deletions global.yaml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,9 @@ global:
text_fill_color: '#124'
text_size: 12px

// following colors are in most cases overwritten
// ones that are used are defined in streetcomplete-light-style.yaml
// and streetcomplete-dark-style.yaml files
railway_color: '#99a'
road_color: '#fff'
highway_color: '#fa8'
Expand Down

16 comments on commit 5dcfd32

@ENT8R
Copy link
Collaborator

@ENT8R ENT8R commented on 5dcfd32 Jun 1, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just for future reference: comments in YAML start with # instead of // (https://yaml.org/spec/1.2/spec.html#id2780069) 😅

@matkoniecz
Copy link
Member Author

@matkoniecz matkoniecz commented on 5dcfd32 Jun 1, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have no idea what went wrong and how I failed to catch this. I was sure that I tested this as usual.

Maybe app install failed for an unrelated reason and I have not noticed this? Because both testing in app and in Tangram Play would reveal this.

Thanks for catching this terribleness and sorry for not catching it myself.

@ENT8R
Copy link
Collaborator

@ENT8R ENT8R commented on 5dcfd32 Jun 1, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No problem at all! I was just wondering why the preview at https://ent8r.github.io/streetcomplete-mapstyle wasn't working as expected 😄

@matkoniecz
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BTW, do you have any idea how to avoid styling outside the map style in streetcomplete/StreetComplete#1877 ?

I tried to both increase text as user zoom in and apply system text scaling and ended with an ugly hack.

@ENT8R
Copy link
Collaborator

@ENT8R ENT8R commented on 5dcfd32 Jun 2, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The following might work in any case:

text_size_scaling: 1
text_size: |
    function() {
        var size = 13;
        if ($zoom >= 18 && $zoom < 19) {
            size = 15;
        } else if ($zoom >= 19) {
            size = 17;
        }
        return size * global.text_size_scaling;
    }

Anyway, I'm currently experimenting a little bit with Tangram Play and will tell you if I found something new, maybe an easier solution...

@matkoniecz
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ooo, so function was supposed to return a number - not a stop array! That is great!

@ENT8R
Copy link
Collaborator

@ENT8R ENT8R commented on 5dcfd32 Jun 2, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know if it would work in any way with a stop array instead of a simple number, but testing it with something like the following led to no success, so it might be true that functions must return a number and no array...

text_size: |
    function() {
        return [
            [17, 13 * global.text_size_scaling],
            [18, 15 * global.text_size_scaling],
            [19, 17 * global.text_size_scaling]
        ];
    }

The following does not work because it seems that calculations (like a multiplication) are only possible in the context of a function

text_size: [
    [17, 13 * global.text_size_scaling],
    [18, 15 * global.text_size_scaling],
    [19, 17 * global.text_size_scaling]
]

So the only solution that seems to be working is the one I posted above.

@matkoniecz
Copy link
Member Author

@matkoniecz matkoniecz commented on 5dcfd32 Jun 2, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ENT8R Are you sure that it is actually working? From what I see it is computed once and not recalculated, so changing zoom level is not changing text size. I tested it in Tangram Play and in a compiled app.

@ENT8R
Copy link
Collaborator

@ENT8R ENT8R commented on 5dcfd32 Jun 2, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For me it works even without reloading the page:

It is indeed only visible if you zoom in very much (breakpoints at $zoom = 18 and $zoom = 19). Would you recommend other breakpoints?

@westnordost
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why breakpoints at all though and not just a function?

function() {
        return (13 + Math.max(0, $zoom - 16)) * global.text_size_scaling;
    }

@westnordost
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Though, road widths are also defined with stop positions, so maybe it is better if text scales when they scale.

https://github.com/ENT8R/streetcomplete-mapstyle/blob/master/layers/roads.yaml#L59

@matkoniecz
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why breakpoints at all though and not just a function?

Solely because breakpoints was the first viable solution that I found (there was no other reason for that).

@matkoniecz
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Though, road widths are also defined with stop positions, so maybe it is better if text scales when they scale.

road widths are linearly scaled between specified breakpoints, there are no sudden jumps. If smooth text scaling works then it is almost certainly preferable.

@matkoniecz
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it is actually working

Wonderful. In Android App it silently fails, requires "px" string to be attached to the returned value. Then it works.

@matkoniecz
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why breakpoints at all though and not just a function?

It appears to be quantized to full zoom levels anyway.

@westnordost
Copy link
Member

@westnordost westnordost commented on 5dcfd32 Jun 2, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, $zoom is unfortunately only full zoom levels. If not, it could have been used to display the actual width of a road, see tangrams/tangram-es#2144

Please sign in to comment.