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

[feature] Add asset icons (desktop mode) #25

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

Conversation

6r1d
Copy link
Contributor

@6r1d 6r1d commented Nov 24, 2022

As told in the title, this PR adds new icons to the "Assets" tab. The screenshot below demonstrates the icons randomly assigned to the asset names, but the PR will match them by the name, case-insensitive. The icons are optimized with svgo, so they take less space when the page is loading.

Thanks to Marina for the icons!

Screenshot

Copy link
Contributor

@0x009922 0x009922 left a comment

Choose a reason for hiding this comment

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

Despite all the comments, I don't like the general idea of that PR. Iroha doesn't have any hardcode related to the tokens (i.e. XOR, VAL etc) presented in this PR. Thus, I think it is quite an unreasonable idea to make these assets hardcoded in the Explorer frontend.

Comment on lines +33 to +41
const { name } = toRefs(props);

let nameLower = '';

const init = async () => {
nameLower = name.value.toLowerCase();
};

init();
Copy link
Contributor

Choose a reason for hiding this comment

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

That is not how we write Vue applications )

That's how:

import { computed } from 'vue'

const nameLower = computed(() => props.name.toLowerCase())

Please read the Computed Properties section (with enabled "Composition" toggle).

Comment on lines +4 to +10
<XOR v-if="nameLower=='xor'" />
<VAL v-else-if="nameLower=='val'" />
<ETH v-else-if="nameLower=='eth'" />
<IrohaIcon v-else-if="nameLower=='iroha'" />
<Soshiba v-else-if="nameLower=='soshiba'" />
<Pswap v-else-if="nameLower=='pswap'" />
<NaIcon v-else />
Copy link
Contributor

Choose a reason for hiding this comment

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

Written in JS, it could be less verbose:

const ICONS = {
  xor: XOR,
  // ...
}

const Icon = computed(() => ICONS[nameLower.value] ?? NaIcon)
<template>
  <component :is="Icon" />
</template>

Comment on lines +49 to +53
display: grid;
grid-gap: size(1);
align-items: center;
grid-auto-flow: column;
grid-auto-columns: auto;
Copy link
Contributor

Choose a reason for hiding this comment

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

.asset div has the only one element inside. grid-auto-flow, grid-auto-columns and grip-gap are useless.

&__icon {
width: size(4);
height: size(4);
border-radius: 50%;
Copy link
Contributor

Choose a reason for hiding this comment

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

border-radius: 50% might produce an oval shape if width is not equal to height. Consider using an absolute value (e.g. 99px) for the best result.

Comment on lines +18 to +24
import XOR from '~icons/assets/XOR.svg';
import VAL from '~icons/assets/VAL.svg';
import ETH from '~icons/assets/ETH.svg';
import IrohaIcon from '~icons/assets/iroha_icon.svg';
import Soshiba from '~icons/assets/soshiba.svg';
import Pswap from '~icons/assets/pswap.svg';
import NaIcon from '~icons/assets/na.svg';
Copy link
Contributor

Choose a reason for hiding this comment

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

It might make sense to import these icons lazily, as dynamic imports. Also you can leverage globs to reduce boilerplate.

@6r1d
Copy link
Contributor Author

6r1d commented Dec 2, 2022

Iroha doesn't have any hardcode related to the tokens (i.e. XOR, VAL etc) presented in this PR. Thus, I think it is quite an unreasonable idea to make these assets hardcoded in the Explorer frontend.

Yes, this is only a start. I feel like we should store the icons somewhere, but in the hindsight, I could've made a repo to make them easily available.

On a last meeting regarding the BCE development direction, we've established the idea of storing the icons in IPFS. According to @appetrosyan, IPFS will store the icon SVGs and BCE backend would cache them. Thus, we only need to download the list of icons, cache those icons and make them available. Alternatively, we can have an "options" dialog to add more, although in my opinion it would be unreasonable for quite some time.

More importantly, the list of our icons should be available somewhere. I feel like putting it into IPFS is as good as putting the icons there, and IPNS would allow us to update said list while saving the address.

So, returning to the repo, I imagine one where we store the icons and trigger some CI job on each update to clone it, upload the icons to IPFS and then to change the list file on IPNS.

We'll need one machine to seed the files and react on our CI, though; otherwise we'll lose the icons, but I'll discuss it with our DevOps.

@0x009922
Copy link
Contributor

0x009922 commented Dec 5, 2022

@6r1d, well, it's fine.

Taking into account everything you've said, it seems that these icons doesn't have anything to do with this repo. From the frontend perspective, any icons/logos it will display will be received from the backend. How those icons will be stored/cached/updated - that's out of scope of the frontend repo.

@KhushiTayal
Copy link

KhushiTayal commented May 8, 2023

Hello @0x009922 ,
I am interested on working on this project under Hyperledger mentorship, can you please guide me how can I get started.

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.

3 participants