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.

Sign up for GitHub

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

Jump to bottom

Add satellite #9468

Merged
merged 26 commits into from
Nov 15, 2023
Merged

Add satellite #9468

merged 26 commits into from
Nov 15, 2023

Conversation

iagocarmona
Copy link
Contributor

satellite-preview

Issue: closes #9462

Similarweb rank:

Checklist

  • I updated the JSON data in _data/simple-icons.json
  • I optimized the icon with SVGO or SVGOMG
  • The SVG viewbox is 0 0 24 24

@github-actions github-actions bot added the new icon Issues or pull requests for adding a new icon label Sep 1, 2023
adamrusted
adamrusted requested changes Sep 3, 2023
View reviewed changes
Copy link
Member

@adamrusted adamrusted left a comment

Choose a reason for hiding this comment

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

Hey @iagocarmona - thanks for the submission!
Comparing your SVG (black) to the source (red) I'm seeing that yours doesn't fill the 24x24 viewport. Could you resize the icon, ensuring that the longest side is exactly 24px?
I'd also suggest we change the source to https://www.satellite.me as that's where the logo is easiest to find.

@LitoMore
Copy link
Member

LitoMore commented Sep 4, 2023

This is weird. This one should fail when the lining stage.

@adamrusted
Copy link
Member

@LitoMore - I've spotted a couple of PRs recently where the linter hasn't caught them. Potentially one to investigate...

@LitoMore
Copy link
Member

LitoMore commented Sep 4, 2023

@adamrusted OK, I'm checking right now.

@LitoMore LitoMore mentioned this pull request Sep 4, 2023
Merged
9 tasks
@adamrusted adamrusted reopened this Sep 4, 2023
@adamrusted
Copy link
Member

There we go, thanks for the fast turn-around on the fix @LitoMore 🎉

@iagocarmona
Copy link
Contributor Author

I'm trying to solve the lint error:

 x icon-size Size of <path> must be exactly 24 in one dimension; the size is currently 19.727 x 22.191
 x icon-centered <path> must be centered at (12, 12); the center is currently (11.59, 12.819)

but I have no Idea how to solve this... someone can help me?
@adamrusted

@adamrusted
Copy link
Member

So that lint error is saying that although the icon canvas is the correct size, the icon itself is not 24px in it's longest dimension. At a glance, I would say the height should be 24px, and if you lock the aspect ratio (in software like Inkscape) the width should auto-adjust to suit. Then you'll just need to center the path on the canvas - and it should be good to go!

@iagocarmona
Copy link
Contributor Author

@adamrusted I think that now it's everthing OK. It's ready to merge! Please review it.

LitoMore
LitoMore requested changes Sep 12, 2023
View reviewed changes
Copy link
Member

@LitoMore LitoMore left a comment

Choose a reason for hiding this comment

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

CleanShot 2023-09-12 at 14 41 31@2x

Please don't stretch the shape. You should keep its aspect ratio.

@iagocarmona
Copy link
Contributor Author

iagocarmona commented Sep 12, 2023 •
edited

@LitoMore, Where did you used the icon to stay like this? I pasted the path on simple icons preview and its looks ok.
simpleicons

@LitoMore
Copy link
Member

LitoMore commented Sep 12, 2023 •
edited

@iagocarmona See the How do I review differences section at #9351.

@iagocarmona
Copy link
Contributor Author

@LitoMore I resized, but the icon dont fit exactly on viewbox, this is the preview:

simpleicons

Should I resize to fit in ?

@iagocarmona
Copy link
Contributor Author

@LitoMore Please, can you review it ? Should I resize the icon to fit on viewbox ?

@LitoMore
Copy link
Member

LitoMore commented Oct 3, 2023

@iagocarmona Yes. Please resize it to fit the viewbox.

@iagocarmona
Copy link
Contributor Author

This is the preview now!

simpleicons

@iagocarmona
Copy link
Contributor Author

@LitoMore Now i think it's ready to merge! Please take a look.

@adamrusted adamrusted removed the request for review from LitoMore November 15, 2023 12:09
@adamrusted adamrusted merged commit dff9c9b into simple-icons:develop Nov 15, 2023
3 checks passed
@github-actions github-actions bot mentioned this pull request Nov 19, 2023
Merged
mondeja added a commit that referenced this pull request Nov 19, 2023
@mondeja
Release 17 new icons and 4 updated icons (v9.21.0)
fa40e28 
# New Icons

- Blackmagic Design (#5488)
- Deluge (#7262)
- Diners Club (#9868)
- Fairphone (#8268)
- Hilton Hotels &amp; Resorts (#9820)
- Insta360 (#9546)
- Material for MkDocs (#9851)
- Mermaid (#9808)
- mpv (#7293)
- PDM (#9805)
- PlatformIO (#8938)
- PortSwigger (#9869)
- Rye (#9844)
- Satellite (#9468)
- Tricentis (#9219)
- Webex (#8613)
- Wells Fargo (#8867)

# Updated Icons

- Hack The Box (#9661)
- Read.cv (#9784)
- SQLAlchemy (#9850)
- Thunderbird (#9761)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@LitoMore LitoMore LitoMore requested changes

@adamrusted adamrusted adamrusted approved these changes

Assignees
No one assigned
Labels
new icon Issues or pull requests for adding a new icon
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add Satellite icon
3 participants
@iagocarmona @LitoMore @adamrusted