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

Add icon for public transport shelter #8007

Merged
merged 3 commits into from
Apr 27, 2024
Merged

Conversation

dvdmrtnz
Copy link
Contributor

@dvdmrtnz dvdmrtnz commented Apr 27, 2024


@dvdmrtnz dvdmrtnz requested a review from a team as a code owner April 27, 2024 11:05
@patepelo
Copy link
Contributor

Thank you @dvdmrtnz. One important thing, this is a redesign of an icon from Roentgen. We wanted to start including them and some modifications like this one. For this PR we would have to include the credits. Here is the origin: https://github.com/enzet/Roentgen and maybe consider adding credits for this other source which we would also like to start using: GitHub:sjjb.co.uk

@dvdmrtnz
Copy link
Contributor Author

Added Röntgen copyright notification

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 change in the outdoors style also!

@dvdmrtnz
Copy link
Contributor Author

Please change in the outdoors style also!

Done

@@ -266,6 +266,10 @@ area|z16-[amenity=shelter],
node|z16-[amenity=shelter][shelter_type=lean_to],
node|z16-[amenity=shelter][shelter_type=basic_hut],
{font-size: 12; icon-min-distance: 0;}
node|z13-[amenity=shelter][shelter_type=public_transport],
{icon-image: zero-icon.svg;}
Copy link
Contributor

@pastk pastk Apr 27, 2024

Choose a reason for hiding this comment

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

please avoid using a zero-icon hack (e.g. its invisible but user-selectable still)
please use icon-image: none; instead

Copy link
Contributor

@pastk pastk Apr 27, 2024

Choose a reason for hiding this comment

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

and node|z13[amenity=... is enough as we need to reset it for z13 only
(its just a minor optimization)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. I didn't know that you could set the icon-image to none

Copy link
Contributor

Choose a reason for hiding this comment

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

yeap its a relatively new improvement :)
need to disable the caption also..

@dvdmrtnz dvdmrtnz force-pushed the shelter branch 2 times, most recently from 38ab2e8 to 3c59438 Compare April 27, 2024 16:33
@@ -978,7 +978,7 @@ tourism-artwork-painting # icon z15- (also has captio
=== 1550

amenity-shelter # icon z13- (also has caption(optional) z13-)
amenity-shelter-public_transport # icon z13- (also has caption(optional) z13-)
amenity-shelter-public_transport # icon z14- and caption z13 (also has caption(optional) z14-)
Copy link
Contributor

Choose a reason for hiding this comment

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

also need to add text: none; to remove the z13 caption..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay! Should be good now!

Signed-off-by: David Martinez <47610359+dvdmrtnz@users.noreply.github.com>
Signed-off-by: David Martinez <47610359+dvdmrtnz@users.noreply.github.com>
Signed-off-by: David Martinez <47610359+dvdmrtnz@users.noreply.github.com>
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
And thanks for your patience!

@pastk pastk requested a review from vng April 27, 2024 18:01
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! Wild shelters are the same as in the master now, right?

@dvdmrtnz
Copy link
Contributor Author

Thanks! Wild shelters are the same as in the master now, right?

Yes. This only changes shelters tagged with shelter_type=public_transport.

@biodranik biodranik merged commit 909385c into organicmaps:master Apr 27, 2024
15 checks passed
@dvdmrtnz dvdmrtnz deleted the shelter branch April 28, 2024 07:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants