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

Moonbeam/Moonriver update logos and colors #7566

Merged
merged 8 commits into from
Oct 14, 2024

Conversation

albertov19
Copy link
Contributor

@albertov19 albertov19 commented Aug 8, 2024

βœ… Checklist

  • npx changeset was attached.
  • Covered by automatic tests.
  • Impact of the changes:

πŸ“ Description

Updated the Moonbeam/Moonriver logos and colors to follow the new Moonbeam/Moonriver brand guidelines

🧐 Checklist for the PR Reviewers

  • The code aligns with the requirements described in the linked JIRA or GitHub issue.
  • The PR description clearly documents the changes made and explains any technical trade-offs or design decisions.
  • There are no undocumented trade-offs, technical debt, or maintainability issues.
  • The PR has been tested thoroughly, and any potential edge cases have been considered and handled.
  • Any new dependencies have been justified and documented.
  • Performance considerations have been taken into account. (changes have been profiled or benchmarked if necessary)

@albertov19 albertov19 requested review from a team as code owners August 8, 2024 20:43
Copy link

vercel bot commented Aug 8, 2024

The latest updates on your projects. Learn more about Vercel for Git β†—οΈŽ

Name Status Preview Comments Updated (UTC)
web-tools βœ… Ready (Inspect) Visit Preview πŸ’¬ Add feedback Oct 9, 2024 5:50am
3 Skipped Deployments
Name Status Preview Comments Updated (UTC)
ledger-live-docs ⬜️ Ignored (Inspect) Visit Preview Oct 9, 2024 5:50am
native-ui-storybook ⬜️ Ignored (Inspect) Visit Preview Oct 9, 2024 5:50am
react-ui-storybook ⬜️ Ignored (Inspect) Visit Preview Oct 9, 2024 5:50am

Copy link

vercel bot commented Aug 8, 2024

@albertov19 is attempting to deploy a commit to the LedgerHQ Team on Vercel.

A member of the Team first needs to authorize it.

@live-github-bot live-github-bot bot added desktop Has changes in LLD ui Has changes in the design system library ledgerjs Has changes in the ledgerjs open source libs fork Pull request base branch comes from a fork. labels Aug 8, 2024
@albertov19
Copy link
Contributor Author

Hey @ypolishchuk-ledger any updates here πŸ˜„ thanks in advance

Copy link

github-actions bot commented Sep 7, 2024

There as been no activity on this PR for the last 14 days. Please consider closing this PR.

@github-actions github-actions bot added the Stale label Sep 7, 2024
@albertov19
Copy link
Contributor Author

Waiting on review from the Ledger Team :)

@github-actions github-actions bot removed the Stale label Sep 10, 2024
@albertov19
Copy link
Contributor Author

Hey all - any updates here? Thanks in advance

@Wozacosta
Copy link
Contributor

Hey there, the svgs you submitted have some extra tags that make them not render (at all).

SCR-20240916-rrmy

Could you maybe try to export them as plain svg (Im guessing you used inkscape or something similar as there are sodipopi tags in the svg https://wiki.inkscape.org/wiki/Inkscape_SVG_vs._plain_SVG).

At the very least for Moonriver, as I was able to clean the Moonbeam svg and make it display properly.

FYI you can use this tool to validate the svgs beforehand without having to build Ledger Live: https://live.ledger.tools/svg-icons.
It catches common tags are missing attributes that could result in the logos not displaying properly.

VicAlbr
VicAlbr previously approved these changes Sep 18, 2024
@albertov19
Copy link
Contributor Author

@Wozacosta I pushed new SVGs following your suggestions, thanks!

@VicAlbr @bharamboure-ledger please check again πŸ™

@Wozacosta
Copy link
Contributor

Checked again, didn't display correctly, here's the versions I modified that make them displayable in desktop:

MOVR
GLMR

@albertov19
Copy link
Contributor Author

Ah @Wozacosta but then the checker is not correct. Your SVGS have just a viewBox and an image tag that has the base64 content while the ones I've been working with have the geometric definitions with circle and path, etc. Nevertheless, your image does not pass the provided SVG checker tool.

Unsure how to proceed as the latest images I tested pass the checker.

@albertov19
Copy link
Contributor Author

@Wozacosta Hey, I hope you are well. Any thoughts on my post above? Would love to get this merged

@Wozacosta
Copy link
Contributor

Hey @albertov19, the SVG checker tool is mainly used for a quick overview of potential errors, but it not passing doesn't necessarily means the SVG won't be displayed.
Have you tried exporting your svgs as plain svgs (or even plain pngs) ?

I'll try to fit in some time to tests those then, but it would help if you're able to test how they're displayed in Ledger Live.

@albertov19
Copy link
Contributor Author

Hey @Wozacosta - I've pushed another commit that should solve all issues! The problem I was having was that I was uploading a <circle> that enclosed the icon, so Ledger was recoloring the entire symbol circle.

The last SVGs are OK in Ledger's SVG checker, but I also built Ledger Live locally with these SVGs, and they show fine πŸ˜„

It was definitely a learning experience haha, hope this helps getting this PR merged πŸš€

image
image

@Wozacosta
Copy link
Contributor

Wozacosta commented Oct 7, 2024

Amazing work, Im testing it on mobile and if that's good I'll approve!

Edit: svgs can be a pain sometimes.. yeah. Glad you kept trying!

@Wozacosta
Copy link
Contributor

Wozacosta commented Oct 7, 2024

Mobile looks good:

image image

Wozacosta
Wozacosta previously approved these changes Oct 7, 2024
@Wozacosta
Copy link
Contributor

Don't forget to rebase your PR, somes failing tests will be successful then.

@albertov19
Copy link
Contributor Author

albertov19 commented Oct 9, 2024

@Wozacosta, I noticed a minimal margin on mobile. I don't have an easy way to test the mobile version, but I bumped a version with larger margins that looks better on the desktop.

That is why I submitted a new commit and your approval was dismissed

@Wozacosta Wozacosta merged commit f275f48 into LedgerHQ:develop Oct 14, 2024
30 of 32 checks passed
@albertov19 albertov19 deleted the moonbeam/update-logos branch October 15, 2024 13:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
desktop Has changes in LLD fork Pull request base branch comes from a fork. ledgerjs Has changes in the ledgerjs open source libs ui Has changes in the design system library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants