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

[styles] attraction=animal as a standalone tag #7716

Merged
merged 5 commits into from
Apr 4, 2024

Conversation

map-per
Copy link
Member

@map-per map-per commented Mar 28, 2024

closes #5911

attraction=* (https://wiki.openstreetmap.org/wiki/Key:attraction) is not a sub key of tourism=attraction (https://wiki.openstreetmap.org/wiki/Tag:tourism%3Dattraction) .
Therefore the style was modified to render attraction=animal without tourism=attraction.

before now
Bildschirmfoto vom 2024-03-28 12-45-18 Bildschirmfoto vom 2024-03-28 19-31-21
before now
Bildschirmfoto vom 2024-03-28 19-47-06 Bildschirmfoto vom 2024-03-28 19-32-49

(also removed the priority values for tourism-attraction-specified, as there is no style specified)

@map-per map-per requested a review from a team as a code owner March 28, 2024 15:07
@RedAuburn
Copy link
Sponsor Member

Nice 👍️ maybe it would be better if we remove the circle from the icon, and use the brown for the giraffe?

@map-per map-per added Icons Map and app icons Styles Map drawing styles labels Mar 28, 2024
@map-per
Copy link
Member Author

map-per commented Mar 28, 2024

Nice 👍️ maybe it would be better if we remove the circle from the icon, and use the brown for the giraffe?

Good idea, did it

Copy link
Member

@biodranik biodranik left a comment

Choose a reason for hiding this comment

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

Thanks! Maybe some day we can make separate icons for each animal )

@pastk PTAL

Copy link
Contributor

@pastk pastk left a comment

Choose a reason for hiding this comment

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

Please amend types_strings.txt accordingly and regenerate strings.

Also you may want to add attraction-animal into editor.config :)

data/mapcss-mapping.csv Outdated Show resolved Hide resolved
data/styles/clear/include/Icons.mapcss Outdated Show resolved Hide resolved
@map-per
Copy link
Member Author

map-per commented Mar 29, 2024

Thanks for the review! I should have checked the list in STYLES.md to not forget that much.
Fixed all problems.

data/mapcss-mapping.csv Outdated Show resolved Hide resolved
data/strings/types_strings.txt Outdated Show resolved Hide resolved
@map-per
Copy link
Member Author

map-per commented Mar 30, 2024

Updated the PR

data/mapcss-mapping.csv Outdated Show resolved Hide resolved
@map-per map-per force-pushed the attraction branch 2 times, most recently from acbe8ab to 96d8641 Compare March 31, 2024 19:22
Copy link
Contributor

@pastk pastk left a comment

Choose a reason for hiding this comment

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

LGTM
Many thanks!

@pastk
Copy link
Contributor

pastk commented Mar 31, 2024

Let's merge after release!

@map-per map-per requested a review from a team April 1, 2024 08:45
data/categories.txt Outdated Show resolved Hide resolved
Copy link
Contributor

@jimcarst jimcarst left a comment

Choose a reason for hiding this comment

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

nl OK

@NeatNit
Copy link
Contributor

NeatNit commented Apr 1, 2024

he okay too.

@rtsisyk
Copy link
Contributor

rtsisyk commented Apr 4, 2024

@pastk @vng merging?

@map-per
Copy link
Member Author

map-per commented Apr 4, 2024

Thanks for the reviews, I updated the strings.

I explicitly defined es-MX and pt-rBR as, for some reason, the ios strings used the tourism.attraction string when I left them out.

From my side this is ready to be merge now.

data/strings/types_strings.txt Outdated Show resolved Hide resolved
data/strings/types_strings.txt Outdated Show resolved Hide resolved
@pastk
Copy link
Contributor

pastk commented Apr 4, 2024

I explicitly defined es-MX and pt-rBR as, for some reason, the ios strings used the tourism.attraction string when I left them out.

Its likely an issue with your twine version, please see my comments above.

map-per added 4 commits April 4, 2024 16:33
Signed-off-by: map-per <map-per@gmx.de>
Signed-off-by: map-per <map-per@gmx.de>
Signed-off-by: map-per <map-per@gmx.de>
Signed-off-by: map-per <map-per@gmx.de>
@map-per
Copy link
Member Author

map-per commented Apr 4, 2024

Thanks, I followed the steps, but that did not help. I now cleaned up the wrong translation files by hand.

data/categories.txt Outdated Show resolved Hide resolved
Copy link
Contributor

@matheusgomesms matheusgomesms left a comment

Choose a reason for hiding this comment

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

This r-BR is wrong, and is duplicate of pt, so no need to add it.

@map-per
Copy link
Member Author

map-per commented Apr 4, 2024

Thanks, I overlooked the lines in categories.txt

@pastk
Copy link
Contributor

pastk commented Apr 4, 2024

Thanks, I followed the steps, but that did not help. I now cleaned up the wrong translation files by hand.

It has helped actually!
Now there are no unrelated strings changes in iphone/Maps/LocalizedStrings/es-MX.lproj/Localizable.strings

Signed-off-by: map-per <map-per@gmx.de>
@map-per
Copy link
Member Author

map-per commented Apr 4, 2024

The unrelated strings changes in iphone/Maps/LocalizedStrings/es-MX.lproj/Localizable.strings are still there, I just removed them by hand.

Copy link
Contributor

@pastk pastk left a comment

Choose a reason for hiding this comment

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

LGTM
Thanks for your patience!

@pastk
Copy link
Contributor

pastk commented Apr 4, 2024

The unrelated strings changes in iphone/Maps/LocalizedStrings/es-MX.lproj/Localizable.strings are still there, I just removed them by hand.

Its ok for now, but better to solve it properly as those sudden changes amongst many localization files are easy to miss in a review..

@RedAuburn how did you fix it on your system?

@map-per
Copy link
Member Author

map-per commented Apr 4, 2024

Thanks for your patience!

Thanks for your help!

@RedAuburn
Copy link
Sponsor Member

@RedAuburn how did you fix it on your system?

i... didn't yet 😅 i tried to find a fix but couldn't, so i just manually remove the replaced es-MX strings

@pastk pastk merged commit ad405b6 into organicmaps:master Apr 4, 2024
15 of 16 checks passed
@map-per map-per deleted the attraction branch April 4, 2024 18:41
pastk added a commit that referenced this pull request Apr 9, 2024
Signed-off-by: Konstantin Pastbin <konstantin.pastbin@gmail.com>
vng pushed a commit that referenced this pull request Apr 10, 2024
Signed-off-by: Konstantin Pastbin <konstantin.pastbin@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Icons Map and app icons Styles Map drawing styles
Projects
None yet
Development

Successfully merging this pull request may close these issues.

use attraction=* as a standalone tag