- Notifications
You must be signed in to change notification settings - Fork5
Add logo and benchmarks to README#60
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
codecov-commenter commentedApr 15, 2023 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Codecov ReportPatch and project coverage have no change.
📣 This organization is not using Codecov’sGitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories.Learn more Additional details and impacted files@@ Coverage Diff @@## main #60 +/- ##======================================= Coverage 71.62% 71.62% ======================================= Files 89 89 Lines 3461 3461 Branches 642 642 ======================================= Hits 2479 2479 Misses 783 783 Partials 199 199 Help us with your feedback. Take ten seconds to tell ushow you rate us. Have a feature suggestion?Share it here. ☔ View full report in Codecov by Sentry. |
SultanOrazbayev left a comment
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 one also looks great, Erik! :)
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.
It's a small thing, but the last letter in "graphblas" is a bit cropped out. I think it still works, though, so not a serious issue.
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.
Really? I don't think I see that. Can you take a screenshot of what you are seeing?
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 checked Chrome and Safari, both look like this for me...
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.
And now that I'm looking at it again, the last s in algorithms is also gone... Interesting.
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 tried playing around with zoom to see if that would make a diff, but nope.
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 see this now (on my macbook); it renders fine for me on Windows where I created it. Thanks for letting me know!
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.
Okay, I think the logo should be fixed now if you'd like to take a look to verify.
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.
To recap testing on my end: looks good on MacOS and Windows, looks truncated on Ubuntu.
SultanOrazbayev commentedApr 21, 2023 via email
nope looks the same …On Sat, 22 Apr 2023 at 03:39, Erik Welch ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ On docs/_static/img/logo-name-medium.svg <#60 (comment)> : Okay, I think the logo should be fixed now if you'd like to take a look to verify. — Reply to this email directly, view it on GitHub <#60 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AE2FWESMMOA7WNUKCUXOEUTXCL5A3ANCNFSM6AAAAAAW7UPWYM> . You are receiving this because your review was requested.Message ID: ***@***.*** com> |
eriknw commentedApr 21, 2023
Dope! Thanks for checking. Jim and I have learned that the font looks very different on different systems (Windows, Mac, Android, etc). So, I think@jim22k is going to convert all the svg logos to PNGs (Jim, don't forget to minimize the PNGs--such as fromhttps://compresspng.com/ andhttps://tinypng.com/ --to improve page load times!). |
jim22k commentedApr 22, 2023
I switched from svg to png. This bumps the logo size from 1k to 35k, but I think it's worth it to avoid the need to grab fonts from the local machine, which leads to different looks and widths. PNG will always be consistent. |
SultanOrazbayev left a comment
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.
Looks good! Interesting the file size is so large (35K), but small enough to not be an issue.
eriknw commentedApr 22, 2023
Thanks for converting the svg font to paths@jim22k. I minified the svg (that you converted the font into paths) withhttps://vecta.io/nano and deleted the png, b/c the svg looks better. I think this is good to go in 🎉 Now let's do the same for the svgs in |
This is a companion PR topython-graphblas/python-graphblas#432
I copied the logo from that PR and manually modified it.