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/delegation #132

Open
wants to merge 5 commits into
base: dev
Choose a base branch
from
Open

feature/delegation #132

wants to merge 5 commits into from

Conversation

Sorizen
Copy link
Collaborator

@Sorizen Sorizen commented Dec 5, 2024

No description provided.

Copy link

vercel bot commented Dec 5, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
dash-board ❌ Failed (Inspect) Dec 30, 2024 3:55am

</script>

<style scoped lang="scss">
.table-sorting-header-column {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why column if it is in fact a row?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Component name is rather too long. Also word Column doesn't really make sense to me, table header is generally a row.

I'd rather make it just TableHeader with optional sorting

@@ -0,0 +1,13 @@
<template>
<div class="delegation">
Copy link
Collaborator

Choose a reason for hiding this comment

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

since it is page a component, let's make it more semantic: <div> --> <main>
unless everything is wrapped with <main> in app layout

Also if app follows classes naming convention based on the component name - I guess it should be delegation-page

import { bus, BUS_EVENTS, ErrorHandler, getEthExplorerTxUrl } from '@/helpers'
import { DELEGATION_RIGHTS } from '@/enums'

const MIN_DELEGATION_AMOUNT = 0.000001
Copy link
Collaborator

Choose a reason for hiding this comment

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

form fields validation consts is better to store in one place in case you'll ever need to reuse them.

example:

export const FIELD_LENGTH = {
    contractName: {
        min: 3,
        max: 64,
    },
    addressList: {
        min: 1,
        max: Number.MAX_SAFE_INTEGER,
    },
    setName: {
        min: 1,
        max: 100,
    },
    description: {
        min: 0,
        max: 300,
    },
} as const

export const FIELD_VALUE = {
    tokenThreshold: {
        min: 1,
        default: 50_000,
        max: Number.MAX_SAFE_INTEGER,
    },
} as const

Comment on lines +6 to +7
@mouseenter="showDelegateButton"
@mouseleave="hideDelegateButton"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is your app supposed to work on touch screen devices?

If yes - everything that happens here on mouse events won't work on those devices

const isDelegateModalShown = ref(false)

const username = computed(() => MOCKED_USERNAMES[props.user.address] ?? '')
const isYou = computed(() => props.user.address === web3ProvidersStore.address)
Copy link
Collaborator

Choose a reason for hiding this comment

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

isYou --> isConnectedWallet

@@ -0,0 +1,130 @@
<template>
<div class="delegation-providers-list">
Copy link
Collaborator

Choose a reason for hiding this comment

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

if it is a list and it looks like list it's better to use semantically matching html elements (<ul>, <ol>, <li>)

@@ -0,0 +1,46 @@
<template>
<div class="delegator-info-page">
Copy link
Collaborator

Choose a reason for hiding this comment

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

same as above
<div> --> <main>

const { t } = useI18n()
const route = useRoute()
const web3ProvidersStore = useWeb3ProvidersStore()
const isYou = computed(
Copy link
Collaborator

Choose a reason for hiding this comment

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

isYou --> isConnectedWallet

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