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

Update brand assets #3610

Merged
merged 4 commits into from
Jan 31, 2022
Merged

Conversation

sweetpea22
Copy link
Contributor

Motivation

updating the assets to make experience of lodestar more awesome. kept file names the same so hopefully no breaking changes.

@codeclimate
Copy link

codeclimate bot commented Jan 12, 2022

Code Climate has analyzed commit c896f69 and detected 0 issues on this pull request.

View more on Code Climate.

@codecov
Copy link

codecov bot commented Jan 12, 2022

Codecov Report

Merging #3610 (a65293d) into master (b384249) will decrease coverage by 0.29%.
The diff coverage is n/a.

❗ Current head a65293d differs from pull request most recent head c896f69. Consider uploading reports for the commit c896f69 to get more accurate results

@@            Coverage Diff             @@
##           master    #3610      +/-   ##
==========================================
- Coverage   37.50%   37.20%   -0.30%     
==========================================
  Files         311      322      +11     
  Lines        8357     8717     +360     
  Branches     1295     1349      +54     
==========================================
+ Hits         3134     3243     +109     
- Misses       5074     5332     +258     
+ Partials      149      142       -7     

Copy link
Contributor

@dapplion dapplion left a comment

Choose a reason for hiding this comment

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

Can you please ensure that the files are in similar pixel sizes and KB sizes? Github PRs are now finally very rich in image diff and show huge reduction in pixel sizes but increases in the KB size. Could you compress the PNGs with some compressor like https://compresspng.com/ ?

@sweetpea22
Copy link
Contributor Author

they're the same pixel size and i'll compress again

@dapplion
Copy link
Contributor

they're the same pixel size and i'll compress again

According to the diff

  • assets/lodestar_icon_text_black.png was W: 3505px | H: 930px now is W: 645px | H: 210px
  • assets/lodestar_icon_text_black_stroke.png was W: 3505px | H: 930px now is W: 646px | H: 210px
  • assets/lodestar_icon_text_white.png was W: 3580px | H: 970px and now is W: 645px | H: 210px

@sweetpea22
Copy link
Contributor Author

oooops, read heights as width 😭 i don't think we need the 3000px width since svgs will be readily available and it would save on bundle size, would that be ok with you?

@dapplion
Copy link
Contributor

oooops, read heights as width sob i don't think we need the 3000px width since svgs will be readily available and it would save on bundle size, would that be ok with you?

Can we keep the original sizes with similar compression since the rendered versions are super convenient?

@sweetpea22 sweetpea22 requested a review from dapplion January 26, 2022 02:10
Copy link
Contributor

@dapplion dapplion left a comment

Choose a reason for hiding this comment

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

Thank you so much for re-uploading the assets!

Copy link
Contributor

@dapplion dapplion left a comment

Choose a reason for hiding this comment

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

Sorry the assets/lodestar_icon_text_black_stroke.png asset is missing a white stroke or it won't be legible in Github dark mode. Please if you can add a simple white stroke in similar fashion to the original asset

@sweetpea22 sweetpea22 requested a review from dapplion January 28, 2022 15:53
@wemeetagain wemeetagain merged commit 52032f8 into ChainSafe:master Jan 31, 2022
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

3 participants