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

Svelte kit #7

Draft
wants to merge 49 commits into
base: main
Choose a base branch
from
Draft

Svelte kit #7

wants to merge 49 commits into from

Conversation

acuarica
Copy link
Contributor

@acuarica acuarica commented Dec 6, 2021

No description provided.

@acuarica acuarica requested a review from fede-rodes December 6, 2021 17:59
src/lib/Addresses.svelte Outdated Show resolved Hide resolved
@@ -1,5 +1,8 @@
<script context="module" lang="ts">
export const config = { renToken: "", renPool: "" };
Copy link
Contributor

Choose a reason for hiding this comment

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

not sure if this component should depend on the context, maybe we could just remove the context and expose a prop called addresses:

export let addresses = {[key: string]: string}

Then, from the parent component you'd pass:

addresses = {
  owner: '0x',
  nodeOperator: '0x',
  renToken: '0x',
  renPool: '0x',
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeap, we can do that.

@@ -33,13 +36,12 @@
<tr>
<td class="key">RenToken</td>
<td class="value"
><EtherscanLink address={NETWORK.contracts.REN_TOKEN} /></td
><EtherscanLink address={config.renToken} /></td
Copy link
Contributor

Choose a reason for hiding this comment

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

in order to avoid repeating this <tr><td> maybe we could pass an array of objects or an object as prop and then do a for loop like this:

{#each addresses as {key, value}}
           <tr>
                <td class="key">{key}</td>
                <td class="value"><EtherscanLink address={value} />
            </td>
{/each}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeap, we can do this as well.

import { NETWORK } from "./config";
<script context="module" lang="ts">
export const config: {
etherscan: string | null;
Copy link
Contributor

@fede-rodes fede-rodes Dec 7, 2021

Choose a reason for hiding this comment

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

Same here, I think this component should not depend on the context. Probably the only component that should depend on the context is the page or the section component and then pass all the context values down to the child components as props.

export let etherscanPrefix: string;
export let address: string;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not here. The I wouldn't use etherscanPrefix as a component prop, because this should be the same for every instance. That's why it's at the module level.

@@ -0,0 +1,69 @@
<script lang="ts">
/**
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should rename this component to Alert and pass a variant prop as in the Chip component. Then, when we do <Alert variant="warning" /> (or danger) we get the original Warn component. This way the component could be used to display successful or info events as well

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeap, sounds good to me.

Copy link
Contributor

@fede-rodes fede-rodes Dec 20, 2021

Choose a reason for hiding this comment

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

@acuarica actually, I've been working on the Alert component and I think it'll be way easier and more robust to simply use a component library and forget about writing our own components. It's way too much time consumer and the end result way poorer. Same applies for the images. Have a look at the following resources:
https://project-awesome.org/TheComputerM/awesome-svelte

Copy link
Contributor Author

Choose a reason for hiding this comment

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

*/
export let title: string;

let dismissed = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

In order to avoid double negation (line 21) I would rename this variable to visible or isVisible to emphatize the fact that it can take on boolean values

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I don't mind that much about this one. Here I tried to highlight the use of the alert to be dismissible. visible sounds more like a toggle which is not implemented.

>
<div class="pr-1">
<svg
class="h-8 w-8 text-white fill-current"
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd move all images to the img folder to make this code a bit easier to read and also being able to reuse the images

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeap, that's a good idea.

@@ -0,0 +1,11 @@
import { BigNumber } from '@ethersproject/bignumber'

Copy link
Contributor

@fede-rodes fede-rodes Dec 14, 2021

Choose a reason for hiding this comment

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

Bond as a file name is a bit confusing to me. Why not calling it constants?

Also, I would not place this inside the net folder, probably move it top level inside the lib folder.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see that bond might be confusing. constants is also fine. I used bond since constants is the analogous for utils, which I usually against. But we can rename until we find a better name.

About moving to the top level, this is related to the structure discussion we had. I wouldn't move it, maybe the net name is the problem.


const network = {
renPoolAddr: '0xbf115a5538290d234fa31e917241a58a20a847bc',
renTokenAddr: deployments.kovan.renTokenAddr,
Copy link
Contributor

Choose a reason for hiding this comment

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

We should set the chain based on the currently connected chain, right? I'm referring to this line

deployments.kovan.renTokenAddr

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was just a hardcoded value to make a PoC. That value would depend on the fork that you would start to run the test. The goal here would be to embed hardhat/ganache to start a blockchain when running those tests. But we need to figure out how to do so.

it('changes count when button is clicked', async () => {
const { renToken } = Contracts(network, provider);

let x = await renToken.poolAllowanceFor('0xbF115A5538290D234fA31e917241a58A20a847Bc')
Copy link
Contributor

Choose a reason for hiding this comment

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

In case x is not being reset, we should use const instead

* adding missing dep

* extending lint npm scripts
* setting up carbon components

* removing warn comp

* replacing Chip with Tag comp

* lint

* update Header

* update header

* name change from selectedAddress to account

* adding wallet to header

* revert name from account to selectedAddress

* revert name from account to selectedAddress

* lint

* revert name to selectedAddress
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.

2 participants