Skip to content
This repository has been archived by the owner on Jul 15, 2022. It is now read-only.

add ohm & sohm icons #1454

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

appleseed-iii
Copy link

Context (issues, jira)

NA.
OHM Token already exists here: https://github.com/LedgerHQ/ledgerjs/blob/48577c43ce81e87d09203dc240c608393a48da33/packages/cryptoassets/data/erc20.js#L1730
SOHM Token was added via this form this past week: https://developers.ledger.com/docs/token/erc20-bep20/#2-how-to-get-my-token-listed

Description / Usage

Add Logos for OHM & SOHM tokens.

Expectations

  • Test coverage: The changes of this PR are covered by test. Unit test were added with mocks when depending on a backend/device.
  • No impact: The changes of this PR have ZERO impact on the userland. Meaning, we can use these changes without modifying LLD/LLM at all. It will be a "noop" and the maintainers will be able to bump it without changing anything.

@appleseed-iii appleseed-iii requested a review from a team October 9, 2021 04:12
@vercel
Copy link

vercel bot commented Oct 9, 2021

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/ledgerhq/ledger-live-common/9KjPohMrN6UQ4jB6Tvmi2xYjYpK1
✅ Preview: https://ledger-live-common-git-fork-appleseed-iii-ohm-s-10e443-ledgerhq.vercel.app

<!-- Generator: Adobe Illustrator 25.4.1, SVG Export Plug-In . SVG Version: 6.00 Build 0) -->
<svg version="1.1" id="Layer_1" xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" x="0px" y="0px"
viewBox="0 0 1000 1000" style="enable-background:new 0 0 1000 1000;" xml:space="preserve">
<style type="text/css">
Copy link
Contributor

Choose a reason for hiding this comment

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

hello thanks for the contribution.
We don't accepts any <style> tag or style attribute within a svg icons.
These svg files are parsed in order for them to be consumed by our live app on both mobile and desktop and have to follow a strict format to do so.
try limiting the icon to a single path and a single color aswell to make it flat. 😄

Copy link
Author

Choose a reason for hiding this comment

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

thanks, we are working on updating our icon per your specs ^^^

Copy link
Author

Choose a reason for hiding this comment

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

@LFBarreto we've updated the tokens & added a new token, WSOHM.

Copy link
Author

Choose a reason for hiding this comment

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

@gre gre added the coin-icon label Oct 26, 2021
@@ -0,0 +1,8 @@
<svg version="1.1" id="Layer_1" xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" x="0px" y="0px"
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the contribution. could you merge master in? we have extra test that do some checks to ensure icons are fine. Thanks

Unfortunately, we can't afford to have id=, class= or <style> in these SVG because we will convert them back into react class.

I still see some style and ids in the SOHM and WSOHM files.

@appleseed-iii appleseed-iii requested a review from a team as a code owner April 11, 2022 18:16
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants