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

Add logo for SSV token #1532

Merged
merged 2 commits into from
Jan 4, 2022
Merged

Add logo for SSV token #1532

merged 2 commits into from
Jan 4, 2022

Conversation

BenAffleck
Copy link
Contributor

@BenAffleck BenAffleck commented Nov 22, 2021

Context (issues, jira)

SSV Token support shipped with version v2.35.0 of the Ledger Live App according to this discussion.

Description / Usage

This PR adds the SSV token logo.

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.

SSV Token support shipped with version v2.35.0 of the Ledger Live App.
This PR adds the SSV token logo.
@BenAffleck BenAffleck requested a review from a team November 22, 2021 11:13
@vercel
Copy link

vercel bot commented Nov 22, 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/2Gw3bY8HUPxVED5yYhCe2KbWUi1i
✅ Preview: https://ledger-live-common-git-fork-benaffleck-patch-1-ledgerhq.vercel.app

@BenAffleck
Copy link
Contributor Author

@gre Thanks for labeling the case. Who will do the review, and when will it happen? Thanks.

@BenAffleck
Copy link
Contributor Author

@gre Any update? Thx.

@gre
Copy link
Contributor

gre commented Dec 6, 2021

@LFBarreto can you confirm this one is good? thanks.

@codecov
Copy link

codecov bot commented Dec 17, 2021

Codecov Report

Merging #1532 (299602f) into master (16a69f3) will decrease coverage by 1.79%.
The diff coverage is n/a.

❗ Current head 299602f differs from pull request most recent head a80ee84. Consider uploading reports for the commit a80ee84 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1532      +/-   ##
==========================================
- Coverage   57.23%   55.43%   -1.80%     
==========================================
  Files         438      549     +111     
  Lines       19745    21836    +2091     
  Branches     4691     5319     +628     
==========================================
+ Hits        11301    12105     +804     
- Misses       8365     9682    +1317     
+ Partials       79       49      -30     
Impacted Files Coverage Δ
src/libcore/createAccountFromDevice.ts 8.82% <0.00%> (-91.18%) ⬇️
src/families/elrond/api/apiCalls.ts 14.28% <0.00%> (-82.86%) ⬇️
src/families/algorand/libcore-buildSubAccounts.ts 16.36% <0.00%> (-81.82%) ⬇️
src/families/algorand/libcore-buildOperation.ts 15.62% <0.00%> (-81.25%) ⬇️
src/families/cosmos/libcore-buildOperation.ts 4.83% <0.00%> (-80.65%) ⬇️
src/libcore/buildAccount/index.ts 14.58% <0.00%> (-80.21%) ⬇️
src/families/bitcoin/js-signOperation.ts 20.58% <0.00%> (-77.85%) ⬇️
src/families/cosmos/libcore-mergeOperations.ts 13.63% <0.00%> (-77.28%) ⬇️
src/families/crypto_org/api/sdk.ts 16.85% <0.00%> (-75.29%) ⬇️
src/families/elrond/api/sdk.ts 23.07% <0.00%> (-75.00%) ⬇️
... and 416 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 16a69f3...a80ee84. Read the comment docs.

@BenAffleck
Copy link
Contributor Author

@LFBarreto @gre
Thanks for the approval. One of the build tests failed, but I assume it is unrelated to my PR.
Please have a look and merge. Thanks.

@BenAffleck
Copy link
Contributor Author

Hello @gre,
Could you please merge now? The process is somewhat unclear to me. Let me know if there is something else I have to do. Thx.

@BenAffleck
Copy link
Contributor Author

Hello @LFBarreto @gre, happy New Year to everyone!
Is anybody active on this issue? It takes a surprisingly long time to close such a simple PR. Is there anything I can do to speed things up?

@gre gre merged commit 1323544 into LedgerHQ:master Jan 4, 2022
@BenAffleck
Copy link
Contributor Author

Hello @gre @LFBarreto, how are you guys doing?
I've received the latest ledger-live update today. Unfortunately, the Icon doesn't show up. Since it isn't showing the generic token icon either, I assume the PR wasn't correct. I have no idea how to fix this. Could you please route me in the right direction?

Screenshot taken from Ledger Live 2.37.1:
image
image
image

@BenAffleck
Copy link
Contributor Author

Hello @gre @LFBarreto, how are you guys doing? The problem still exists, even with Ledger Live 2.37.2.
Sorry to bother you, but I need advice. What's wrong with the logo? Please have a look. Thanks.

@BenAffleck
Copy link
Contributor Author

Hello @LFBarreto @gre,
I'm a bit lost here. I don't understand if the problem was solved with #1700 or not? I just downloaded the latest release, but the icon doesn't appear. Can you please shed some light on it, so we can finally unblock it? Thank you.

@BenAffleck
Copy link
Contributor Author

Hello @LFBarreto @gre, please, I need your advice. The logo is still now showing in the latest version. Please let me know what to do next.

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