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

Staking draft #46

Draft
wants to merge 65 commits into
base: master
Choose a base branch
from
Draft

Staking draft #46

wants to merge 65 commits into from

Conversation

neokrept
Copy link
Contributor

@neokrept neokrept commented Apr 6, 2021

No description provided.

Copy link
Member

@danimoh danimoh left a comment

Choose a reason for hiding this comment

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

Overall looking good so far.
Sorry, I didn't really find the time yet to look into it in detail.
I just skimmed over it such that you can already work on some feedback.

Comment on lines 49 to 109
export function numberToLiteral(n: number): string {
// https://stackoverflow.com/questions/5529934/javascript-numbers-to-words
const ones = ['', 'one', 'two', 'three', 'four', 'five', 'six', 'seven', 'eight', 'nine'];
const tens = ['', '', 'twenty', 'thirty', 'forty', 'fifty', 'sixty', 'seventy', 'eighty', 'ninety'];
const teens = ['ten', 'eleven', 'twelve', 'thirteen', 'fourteen', 'fifteen',
'sixteen', 'seventeen', 'eighteen', 'nineteen'];

function convertMillions(num: number): string {
if (num >= 1000000) {
return `${convertMillions(Math.floor(num / 1000000))} million ${convertThousands(num % 1000000)}`;
}
return convertThousands(num);
}

function convertThousands(num: number): string {
if (num >= 1000) return `${convertHundreds(Math.floor(num / 1000))} thousand ${convertHundreds(num % 1000)}`;
return convertHundreds(num);
}

function convertHundreds(num: number): string {
if (num > 99) return `${ones[Math.floor(num / 100)]} hundred ${convertTens(num % 100)}`;
return convertTens(num);
}

function convertTens(num: number): string {
if (num < 10) return ones[num];
if (num >= 10 && num < 20) return teens[num - 10];
return `${tens[Math.floor(num / 10)]} ${ones[num % 10]}`;
}

if (n === 0) return 'zero';
return convertMillions(n);
}

export function numberToLiteralTimes(n: number): string {
const timesTable = ['', 'once', 'twice', 'thrice'];

n = parseInt(n.toString(), 10);
if (n <= 0) throw new Error('Invalid Input! Times number must be positive >= 1!');
if (n < timesTable.length) return timesTable[n];

return `${numberToLiteral(n)} times`;
}
Copy link
Member

Choose a reason for hiding this comment

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

This is missing internationalization.
You can import i18n from i18n-setup.ts and then use its t method.
Using variables is also supported in source strings, which shall be useful here, e.g.:

i18n.t('{millions} million {remainder}', {
    millions: convertMillions(Math.floor(num / 1000000)),
    remainder: convertThousands(num % 1000000),
})

Comment on lines 56 to 77
function convertMillions(num: number): string {
if (num >= 1000000) {
return `${convertMillions(Math.floor(num / 1000000))} million ${convertThousands(num % 1000000)}`;
}
return convertThousands(num);
}

function convertThousands(num: number): string {
if (num >= 1000) return `${convertHundreds(Math.floor(num / 1000))} thousand ${convertHundreds(num % 1000)}`;
return convertHundreds(num);
}

function convertHundreds(num: number): string {
if (num > 99) return `${ones[Math.floor(num / 100)]} hundred ${convertTens(num % 100)}`;
return convertTens(num);
}

function convertTens(num: number): string {
if (num < 10) return ones[num];
if (num >= 10 && num < 20) return teens[num - 10];
return `${tens[Math.floor(num / 10)]} ${ones[num % 10]}`;
}
Copy link
Member

Choose a reason for hiding this comment

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

Defining inner methods is not very elegant as redefines the methods on each invocation of the outer method which can be heavy on the garbage collector.

Comment on lines 4 to 17
<template v-if="page === 1">
<StakeInfoPage @next="page += 1"/>
</template>
<template v-if="page === 2">
<StakeValidatorPage @back="page -= 1" @next="page += 1"/>
</template>
Copy link
Member

Choose a reason for hiding this comment

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

If the template only contains one child node, you can omit the template and apply the v-if directly on the child.

<template>
<div class="validator-filter">
<div class="validator-filter-wrapper">
<button class="validator-switch" :class="state === FilterState.TRUST?'selected':''"
Copy link
Member

Choose a reason for hiding this comment

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

There's a more elegant way to implement conditional style classes:
:class="{ selected: state === FilterState.TRUST }"

Comment on lines 16 to 23
<span class="validator-search" @click="ctrl.stateChange(FilterState.SEARCH)">
<SearchIcon/>
</span>
Copy link
Member

Choose a reason for hiding this comment

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

This should also be a button or something to be usable via keyboard navigation.
Additionally, it should probably have hover and focus styles.

<button class="validator-list-item reset flex-row">
<div class="validator-item-wrapper">
<div class="validator-left">
<component v-bind:is="Icons[Helpers.capitalise(validatorData.icon) + 'Icon']" class="tab"></component>
Copy link
Member

Choose a reason for hiding this comment

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

: can be used as a short form for v-bind:, i.e. :is=...

</div>
</div>
<div :class="validatorData.reward < 2.5?'validator-warning':''" class="validator-item-right">
{{ validatorData.reward.toFixed(1) }} % {{ $t("p.a.") }}
Copy link
Member

Choose a reason for hiding this comment

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

Use variable slots instead:
{{ $t("{reward}% p.a.", { reward: validatorData.reward.toFixed(1) }) }}

Comment on lines 44 to 78
const getPayoutText = (payout: Array<number>) => {
const periods = ['year', 'month', 'week', 'day', 'h'];
let index = 0;
let value = 0;

for (let i = 0; i < payout.length; i++) {
if (payout[i] > 0) {
index = i;
value = payout[i];
break;
}
}

if (index === periods.length - 1) {
return `pays out every ${value}${periods[index]}`;
}
return `pays out ${numberToLiteralTimes(value)} a ${periods[index]}`;
};
Copy link
Member

Choose a reason for hiding this comment

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

Missing internationalization.

export default defineComponent({
setup(props) {
return {
payoutText: getPayoutText(props.validatorData.payout),
Copy link
Member

Choose a reason for hiding this comment

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

This should rather be a computed property to be reactive to data changes.

Comment on lines 80 to 41
validatorData: {
type: Object,
required: true,
},
Copy link
Member

Choose a reason for hiding this comment

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

Might want to make the properties of validatorData separate props to be more specific about which properties are expected.
Or could add a prop validation method that checks for the expected properties.

Copy link
Member

@danimoh danimoh left a comment

Choose a reason for hiding this comment

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

Hello Sabin,
I now found the time to look through your PR in detail.
There are actually still quite some more issues.
However, don't be discouraged by the amount of comments.
Many things are also about our best practices or very opinionated or generally just reflect my personal opinions.
Feel free to comment if you disagree with something.

Generally, please try to follow the designs more closely in the future, as there were quite some deviations.
If you want to propose changes to the designs, please discuss them with our designers.

Also I noticed some unnecessary code leftovers which can be cleaned up like empty blocks or unused / undefined style classes and unnecessary style declarations.
Please look through your changes before forging a commit and perform such cleanups before pushing.

Comment on lines +1 to +5
<template functional>
<svg width="42" height="42" viewBox="0 0 42 42" fill="none" xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink">
<image id="image0" width="42" height="42" xlink:href="data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAACMAAAAjCAMAAAApB0NrAAAACXBIWXMAAE69AABOvQFzamgUAAADAFBMVEVHcExz8KJ866Z86qVt859r9J+D56h766V47aR37qNp9J9/6KZ07qKC6Kdz8KKB6Kdx8KF47aOD56iC56d466OA5qZv8aBy7aCA6ahv8p927aOG8q5t859w+aN076Jx8KGE56mD5ql87KWE5ql47aR86qZ666V37aRv8aCC6KiD5qh37aNq9Z937aRp9J5q9Z9q9J9q9Z9s859q9Z5t8p+E5qlq9J6D5aiD5qmD5qmD56h37aOE56mD5qiD5qhq9J6D56h+6aaD5ql076Jq9Z947KN17qOD56dv8p1p9J5x8KGA6KZr9Z+C5qiE5qlq9Z6B9axu8p+B6Kd65aJw8aBq9Z9p9J9q9Z9p5Jdq9Z9n15Fr8p6C5qdw8J9766V686d76qRt+qN+8qpw8aFz8KJ366KL9LKG7qx/6KaE76yE969w9qJx7qBUdWF67aVmp36A+a5xyJGB8Ktv/6Zs9J9w/6iE7Kpu/aR96KV0x5OD6Kh/5qV57aRgj3Js85937qOA6qeB7Kl68aZ97qdfnXZ2/KlLWVCD6qmH9bCE8q1XkW189albpHZ96aWB6Kdy/aZs55l56qN/9at+76l666RilnV38qVirH516KBx+KNq1pJ1+6dz96V29KV676U+MTlTd2GJ8LCJ87B5151jmXdvyZFs9qB38KSC8qx87KaB86xdjG9jxIdz/6lgm3dZl3Bno3594KJutIhdoXZ3/6py8qJWeGJXf2Zem3Vy9KNz35tcnnRJV056+alRbFtCPD9NY1Z19aVot4Vx2piE/rKM9bNzw5GJ97Fkp31ftH58/q1t+aJoqYCF9a9kyop5/atz05d9+KtowYlx+qRyy5N75aNfknJ51Zt+6qZScV5Tc19Zh2puw45s4Jhjs4F15Z9clHForoJOZlc9LDdYjmxDQUJpxYty15hakm94+Kg/NjtuzZGP+7dwu4xtyY9npn5YmHB866Vmn3tv5p1GSUdq25RisYBIUkxYlW9GSEZGS0hv45tqzI9gp3paiGtOaFhNPK7iAAAAf3RSTlMAE5r8Afz8AgMBEPv+ARWa/e79/fz8/f4MBPz+7f7t7YQqB84z25hUbKD4OiUe6uOaw+/6/l4NUIpF4Yqqs5Ck4Llqkl2e6l4G2rd6QMPoz/528f7XZEms/oj+///////////////////////////////////////////////+vewlkQAAA3NJREFUOMtjYCAFsEABJwwQowmLIjM+RmTAJ4GuiJ1BR99IBgjk5eXl5OQEgUCVD00RO4P92pJl2dnZO3fOnTtt2rymprwjagwcyEo4GHxOs9aZiIkJCIiL80hJiYiIxCV8VAFqRQaGxazRspdnzpkzZ/bBufHx3U0JzFmODLzIxoSWApWc+7F59+7dT3a0tLQ8WDo5gblTHWEbCwOXfiLrsglPvt69+/vP03v3D++6sDp4JXORqhKSMZFAY8p71qzdunXL3/m39+blFRzKD6jgPu4NMwjob+Nl0aej1s/MyUkRP/D01/OmiQnNtdcbMxqDdKFOYmcIBxqT+GEWUM3s6bkPHu+4ODEhqXBJUQb3cU2I1zgYwsqBNq3p6Y9Nzjn4/9Dh/W338xJ6Vy05VlTdV6YNsY3Fv5i1xKRrws7YnNmX2mbE585/OKmid/Li2oY+tkW+EGM0gDYVnzwJMmb2ghcv4w+92rfyY29F55lN0qJsi9xBBvHpnY4+HdGVnBybc3DWnl174+cd2H8nqSKjoXr1xnbhdAcJoEEGpcbRp3vOXQYa0/rk0eHXr5//m78dqKa68+2G9Bi2NAsGBknPcta6uq4J2bE5W3/v2bctN3dB7s8vFyZnZDQurq2KYVuoDDTHpRjiq5yc1s2PlqemTpw4+fb8qauqqzuXHAOas9AcGp0msutnzclJaZu+N7UmL2HSnf1tGzMyK6dmAq2yBYYiO4P2sjoT2Vnr52z5tid1eWrNlISPzftePTzbXnu9LKaKTREUihwMBmvFYmdvviT+/cCMGTU1Uz5evbH6cdvGTRukY9iO2oHDkIVB1yhbbOvnW9tvLth2KX5iwuQr27fdupm7TbRKON2PERJhkgyBWwRyZt+bfr4+dWIe0KqKqw0NZzdsAhqT5gRPQR4hcwRSeF7sWg701MSECmDYNF6vbYf6G5Z+1GcL8Lx81nKqu7t70qRVK1asODu1Mj1GKN0SkRA5GbzO8/DMbdkx/cSJ/Pz379+fmfoJ5G8bpETPzqACtKt+xrulSwuBoBIIRKtiqrQUkTMGB4PzKan6mvj4pKSkIiDIzATGwlEFlAzGycDnthyUq5iZuYFAVFRUWKvMSgI574D8r3HkYgEQZGWd7QCCRYsWvTFFzadAoGTtqsYEBvxgoKnAzkAWYEcHcBkAW25lnC9sM7UAAAAASUVORK5CYII="/>
</svg>
</template>
Copy link
Member

Choose a reason for hiding this comment

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

Not sure whether the validator icons should be components. Being components a bit of boilerplate is added for each icon. Also, they way they're currently imported, they get bundled, inflating the bundle size.
For the mock icons and data, that is certainly okay and you don't need to change it.
But I think the final icons would rather be just images that sit somewhere (maybe not even in the wallet repository) and will be loaded on demand.
The other icons (search icon, star icon, stalking icon) can remain components, that's alright.

@@ -0,0 +1,5 @@
<template functional>
<svg width="42" height="42" viewBox="0 0 42 42" fill="none" xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink">
<image id="image0" width="42" height="42" xlink:href="data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAACMAAAAjCAMAAAApB0NrAAAACXBIWXMAAE69AABOvQFzamgUAAADAFBMVEVHcExz8KJ866Z86qVt859r9J+D56h766V47aR37qNp9J9/6KZ07qKC6Kdz8KKB6Kdx8KF47aOD56iC56d466OA5qZv8aBy7aCA6ahv8p927aOG8q5t859w+aN076Jx8KGE56mD5ql87KWE5ql47aR86qZ666V37aRv8aCC6KiD5qh37aNq9Z937aRp9J5q9Z9q9J9q9Z9s859q9Z5t8p+E5qlq9J6D5aiD5qmD5qmD56h37aOE56mD5qiD5qhq9J6D56h+6aaD5ql076Jq9Z947KN17qOD56dv8p1p9J5x8KGA6KZr9Z+C5qiE5qlq9Z6B9axu8p+B6Kd65aJw8aBq9Z9p9J9q9Z9p5Jdq9Z9n15Fr8p6C5qdw8J9766V686d76qRt+qN+8qpw8aFz8KJ366KL9LKG7qx/6KaE76yE969w9qJx7qBUdWF67aVmp36A+a5xyJGB8Ktv/6Zs9J9w/6iE7Kpu/aR96KV0x5OD6Kh/5qV57aRgj3Js85937qOA6qeB7Kl68aZ97qdfnXZ2/KlLWVCD6qmH9bCE8q1XkW189albpHZ96aWB6Kdy/aZs55l56qN/9at+76l666RilnV38qVirH516KBx+KNq1pJ1+6dz96V29KV676U+MTlTd2GJ8LCJ87B5151jmXdvyZFs9qB38KSC8qx87KaB86xdjG9jxIdz/6lgm3dZl3Bno3594KJutIhdoXZ3/6py8qJWeGJXf2Zem3Vy9KNz35tcnnRJV056+alRbFtCPD9NY1Z19aVot4Vx2piE/rKM9bNzw5GJ97Fkp31ftH58/q1t+aJoqYCF9a9kyop5/atz05d9+KtowYlx+qRyy5N75aNfknJ51Zt+6qZScV5Tc19Zh2puw45s4Jhjs4F15Z9clHForoJOZlc9LDdYjmxDQUJpxYty15hakm94+Kg/NjtuzZGP+7dwu4xtyY9npn5YmHB866Vmn3tv5p1GSUdq25RisYBIUkxYlW9GSEZGS0hv45tqzI9gp3paiGtOaFhNPK7iAAAAf3RSTlMAE5r8Afz8AgMBEPv+ARWa/e79/fz8/f4MBPz+7f7t7YQqB84z25hUbKD4OiUe6uOaw+/6/l4NUIpF4Yqqs5Ck4Llqkl2e6l4G2rd6QMPoz/528f7XZEms/oj+///////////////////////////////////////////////+vewlkQAAA3NJREFUOMtjYCAFsEABJwwQowmLIjM+RmTAJ4GuiJ1BR99IBgjk5eXl5OQEgUCVD00RO4P92pJl2dnZO3fOnTtt2rymprwjagwcyEo4GHxOs9aZiIkJCIiL80hJiYiIxCV8VAFqRQaGxazRspdnzpkzZ/bBufHx3U0JzFmODLzIxoSWApWc+7F59+7dT3a0tLQ8WDo5gblTHWEbCwOXfiLrsglPvt69+/vP03v3D++6sDp4JXORqhKSMZFAY8p71qzdunXL3/m39+blFRzKD6jgPu4NMwjob+Nl0aej1s/MyUkRP/D01/OmiQnNtdcbMxqDdKFOYmcIBxqT+GEWUM3s6bkPHu+4ODEhqXBJUQb3cU2I1zgYwsqBNq3p6Y9Nzjn4/9Dh/W338xJ6Vy05VlTdV6YNsY3Fv5i1xKRrws7YnNmX2mbE585/OKmid/Li2oY+tkW+EGM0gDYVnzwJMmb2ghcv4w+92rfyY29F55lN0qJsi9xBBvHpnY4+HdGVnBybc3DWnl174+cd2H8nqSKjoXr1xnbhdAcJoEEGpcbRp3vOXQYa0/rk0eHXr5//m78dqKa68+2G9Bi2NAsGBknPcta6uq4J2bE5W3/v2bctN3dB7s8vFyZnZDQurq2KYVuoDDTHpRjiq5yc1s2PlqemTpw4+fb8qauqqzuXHAOas9AcGp0msutnzclJaZu+N7UmL2HSnf1tGzMyK6dmAq2yBYYiO4P2sjoT2Vnr52z5tid1eWrNlISPzftePTzbXnu9LKaKTREUihwMBmvFYmdvviT+/cCMGTU1Uz5evbH6cdvGTRukY9iO2oHDkIVB1yhbbOvnW9tvLth2KX5iwuQr27fdupm7TbRKON2PERJhkgyBWwRyZt+bfr4+dWIe0KqKqw0NZzdsAhqT5gRPQR4hcwRSeF7sWg701MSECmDYNF6vbYf6G5Z+1GcL8Lx81nKqu7t70qRVK1asODu1Mj1GKN0SkRA5GbzO8/DMbdkx/cSJ/Pz379+fmfoJ5G8bpETPzqACtKt+xrulSwuBoBIIRKtiqrQUkTMGB4PzKan6mvj4pKSkIiDIzATGwlEFlAzGycDnthyUq5iZuYFAVFRUWKvMSgI574D8r3HkYgEQZGWd7QCCRYsWvTFFzadAoGTtqsYEBvxgoKnAzkAWYEcHcBkAW25lnC9sM7UAAAAASUVORK5CYII="/>
Copy link
Member

Choose a reason for hiding this comment

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

SVG with inline PNG doesn't make too much sense, but it's also just mock data, so okay.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here I didn't have the SVG information in the mock-up.

@@ -0,0 +1,8 @@
<!-- TODO: get latest version from designers -->
Copy link
Member

Choose a reason for hiding this comment

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

You can ask our designer Julian for the properly exported assets.
Sometimes we also just run the assets exported from figma ourselves through a minifier (but the designers prefer to use their exported versions) or run the designer's version through the minifier again to squeeze out some extra bytes.
You can use svgo or the cool web frontend svgomg for minifying svgs.

This also holds for the other non-mock icons: the search icon and the star icon.

Comment on lines +1 to +5
<template functional>
<svg width="42" height="42" viewBox="0 0 42 42" fill="none" xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink">
<image id="image0" width="42" height="42" xlink:href="data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAACMAAAAjCAMAAAApB0NrAAAACXBIWXMAAE69AABOvQFzamgUAAADAFBMVEVHcExz8KJ866Z86qVt859r9J+D56h766V47aR37qNp9J9/6KZ07qKC6Kdz8KKB6Kdx8KF47aOD56iC56d466OA5qZv8aBy7aCA6ahv8p927aOG8q5t859w+aN076Jx8KGE56mD5ql87KWE5ql47aR86qZ666V37aRv8aCC6KiD5qh37aNq9Z937aRp9J5q9Z9q9J9q9Z9s859q9Z5t8p+E5qlq9J6D5aiD5qmD5qmD56h37aOE56mD5qiD5qhq9J6D56h+6aaD5ql076Jq9Z947KN17qOD56dv8p1p9J5x8KGA6KZr9Z+C5qiE5qlq9Z6B9axu8p+B6Kd65aJw8aBq9Z9p9J9q9Z9p5Jdq9Z9n15Fr8p6C5qdw8J9766V686d76qRt+qN+8qpw8aFz8KJ366KL9LKG7qx/6KaE76yE969w9qJx7qBUdWF67aVmp36A+a5xyJGB8Ktv/6Zs9J9w/6iE7Kpu/aR96KV0x5OD6Kh/5qV57aRgj3Js85937qOA6qeB7Kl68aZ97qdfnXZ2/KlLWVCD6qmH9bCE8q1XkW189albpHZ96aWB6Kdy/aZs55l56qN/9at+76l666RilnV38qVirH516KBx+KNq1pJ1+6dz96V29KV676U+MTlTd2GJ8LCJ87B5151jmXdvyZFs9qB38KSC8qx87KaB86xdjG9jxIdz/6lgm3dZl3Bno3594KJutIhdoXZ3/6py8qJWeGJXf2Zem3Vy9KNz35tcnnRJV056+alRbFtCPD9NY1Z19aVot4Vx2piE/rKM9bNzw5GJ97Fkp31ftH58/q1t+aJoqYCF9a9kyop5/atz05d9+KtowYlx+qRyy5N75aNfknJ51Zt+6qZScV5Tc19Zh2puw45s4Jhjs4F15Z9clHForoJOZlc9LDdYjmxDQUJpxYty15hakm94+Kg/NjtuzZGP+7dwu4xtyY9npn5YmHB866Vmn3tv5p1GSUdq25RisYBIUkxYlW9GSEZGS0hv45tqzI9gp3paiGtOaFhNPK7iAAAAf3RSTlMAE5r8Afz8AgMBEPv+ARWa/e79/fz8/f4MBPz+7f7t7YQqB84z25hUbKD4OiUe6uOaw+/6/l4NUIpF4Yqqs5Ck4Llqkl2e6l4G2rd6QMPoz/528f7XZEms/oj+///////////////////////////////////////////////+vewlkQAAA3NJREFUOMtjYCAFsEABJwwQowmLIjM+RmTAJ4GuiJ1BR99IBgjk5eXl5OQEgUCVD00RO4P92pJl2dnZO3fOnTtt2rymprwjagwcyEo4GHxOs9aZiIkJCIiL80hJiYiIxCV8VAFqRQaGxazRspdnzpkzZ/bBufHx3U0JzFmODLzIxoSWApWc+7F59+7dT3a0tLQ8WDo5gblTHWEbCwOXfiLrsglPvt69+/vP03v3D++6sDp4JXORqhKSMZFAY8p71qzdunXL3/m39+blFRzKD6jgPu4NMwjob+Nl0aej1s/MyUkRP/D01/OmiQnNtdcbMxqDdKFOYmcIBxqT+GEWUM3s6bkPHu+4ODEhqXBJUQb3cU2I1zgYwsqBNq3p6Y9Nzjn4/9Dh/W338xJ6Vy05VlTdV6YNsY3Fv5i1xKRrws7YnNmX2mbE585/OKmid/Li2oY+tkW+EGM0gDYVnzwJMmb2ghcv4w+92rfyY29F55lN0qJsi9xBBvHpnY4+HdGVnBybc3DWnl174+cd2H8nqSKjoXr1xnbhdAcJoEEGpcbRp3vOXQYa0/rk0eHXr5//m78dqKa68+2G9Bi2NAsGBknPcta6uq4J2bE5W3/v2bctN3dB7s8vFyZnZDQurq2KYVuoDDTHpRjiq5yc1s2PlqemTpw4+fb8qauqqzuXHAOas9AcGp0msutnzclJaZu+N7UmL2HSnf1tGzMyK6dmAq2yBYYiO4P2sjoT2Vnr52z5tid1eWrNlISPzftePTzbXnu9LKaKTREUihwMBmvFYmdvviT+/cCMGTU1Uz5evbH6cdvGTRukY9iO2oHDkIVB1yhbbOvnW9tvLth2KX5iwuQr27fdupm7TbRKON2PERJhkgyBWwRyZt+bfr4+dWIe0KqKqw0NZzdsAhqT5gRPQR4hcwRSeF7sWg701MSECmDYNF6vbYf6G5Z+1GcL8Lx81nKqu7t70qRVK1asODu1Mj1GKN0SkRA5GbzO8/DMbdkx/cSJ/Pz379+fmfoJ5G8bpETPzqACtKt+xrulSwuBoBIIRKtiqrQUkTMGB4PzKan6mvj4pKSkIiDIzATGwlEFlAzGycDnthyUq5iZuYFAVFRUWKvMSgI574D8r3HkYgEQZGWd7QCCRYsWvTFFzadAoGTtqsYEBvxgoKnAzkAWYEcHcBkAW25lnC9sM7UAAAAASUVORK5CYII="/>
</svg>
</template>
Copy link
Member

Choose a reason for hiding this comment

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

Our folder names are usually lower-case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually in that folder there are 2 more other folders with the same casing (AccountMenu and SwapBalanceBar), was trying to follow this "convention".

Comment on lines 4 to 23
<button class="validator-switch" :class="state === FilterState.TRUST?'selected':''"
@click="ctrl.stateChange(FilterState.TRUST)">
{{ $t('TrustScore') }}
</button>
<button class="validator-switch" :class="state === FilterState.PAYOUT?'selected':''"
@click="ctrl.stateChange(FilterState.PAYOUT)">
{{ $t('Payout Time') }}
</button>
<button class="validator-switch" :class="state === FilterState.REWARD?'selected':''"
@click="ctrl.stateChange(FilterState.REWARD)">
{{ $t('Reward') }}
</button>
<span class="validator-search" @click="ctrl.stateChange(FilterState.SEARCH)">
<SearchIcon/>
</span>
Copy link
Member

Choose a reason for hiding this comment

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

These could be radio buttons to make the behavior semantically more explicit.

scrollbar-width: 1rem;

&::-webkit-scrollbar {
width: 1rem;
Copy link
Member

Choose a reason for hiding this comment

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

It's only .75rem in the designs.

Comment on lines 78 to 90
scrollbar-color: #bcbec9 rgba(0, 0, 0, 0);
scrollbar-width: 1rem;

&::-webkit-scrollbar {
width: 1rem;
}

&::-webkit-scrollbar-track {
background: rgba(0, 0, 0, 0);
}
&::-webkit-scrollbar-thumb {
background-color: #bcbec9;
}
Copy link
Member

Choose a reason for hiding this comment

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

I think you can omit these custom definitions.
@extend %custom-scrollbar should be enough.
(While the scroll bar color defined in %custom-scrollbar seems to be different from the designs here,
I think for consistency reasons it should be used nonetheless.)

Copy link
Contributor Author

@neokrept neokrept Apr 11, 2021

Choose a reason for hiding this comment

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

So we won't be using the transparent background for the scrollbar for this? That is the main difference I think, the thumb rule is probably not necessary here.

display: inline-block;
margin-top: 1rem;
margin-bottom: .25rem;
font-size: 2rem;
Copy link
Member

Choose a reason for hiding this comment

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

Should be the default for nq-text.

background-color: #bcbec9;
}
}
}
Copy link
Member

Choose a reason for hiding this comment

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

The list is missing the fade out effect at the end of the page.
You have a look at AccountSelector in vue-components for implementation inspiration.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Was not sure I was seeing that right in the mock-ups :), thanks for the suggestion.

},
name: 'stake',
props: { modal: true },
meta: { column: Columns.ACCOUNT }, // TODO: investigate usage
Copy link
Member

Choose a reason for hiding this comment

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

@sisou What would be the recommended value here?

Copy link
Member

Choose a reason for hiding this comment

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

I believe we start staking always from the address view? In that case Columns.ADDRESS would be appropriate.

Copy link
Member

@sisou sisou Apr 12, 2021

Choose a reason for hiding this comment

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

The column is used only on mobile (thin) screens to move the view to the respective column. Most modals have Column.DYNAMIC, so they don't move the view when opened. But here I believe the address column is what we want to show. (Setting it to ACCOUNT would always move the view over to the account column (center column) when the modal is opened, which is weird, as its opened from a button on the address column (right column)).

Copy link
Member

@danimoh danimoh left a comment

Choose a reason for hiding this comment

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

This review includes changes up to 91a25e7, so some things might be outdated by your latest changed.
I did not include style issues in this review, but there are many of them.

<Tooltip
class="staking-feature-tip"
preferredPosition="bottom left"
:container="this">
Copy link
Member

Choose a reason for hiding this comment

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

this doesn't work. Use $el instead.

Comment on lines 110 to 121
<div slot="trigger">
<button class="stake" @click="$router.push('/stake')"
@mousedown.prevent
:disabled="(activeCurrency === 'nim' && (!activeAddressInfo || !activeAddressInfo.balance))
|| activeCurrency !== 'nim'"
>
<StakingHeroIcon />
</button>
</div>
<span>
{{ $t('Earn ~304 NIM a month by staking your NIM') }}
</span>
Copy link
Member

Choose a reason for hiding this comment

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

This is deprecated slot syntax.
Instead use:

<template #trigger>

</template>
<template #default>

</template>

Comment on lines 113 to 114
:disabled="(activeCurrency === 'nim' && (!activeAddressInfo || !activeAddressInfo.balance))
|| activeCurrency !== 'nim'"
Copy link
Member

Choose a reason for hiding this comment

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

Condition can be simplified to activeCurrency !== 'nim' || !activeAddressInfo || !activeAddressInfo.balance. Theoretically even activeCurrency !== 'nim' || !activeAddressInfo?.balance but that doesn't seem to be supported by our Vue version yet.

Comment on lines 111 to 117
<button class="stake" @click="$router.push('/stake')"
@mousedown.prevent
:disabled="(activeCurrency === 'nim' && (!activeAddressInfo || !activeAddressInfo.balance))
|| activeCurrency !== 'nim'"
>
<StakingHeroIcon />
</button>
Copy link
Member

Choose a reason for hiding this comment

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

This button should have the regular nq-button-pill green style and hover, focus and disabled behavior.
I suggest implementing this as a regular nq-button-pill green button with adapted size, box-shadows for the rings and the staking plant icon as content, instead of using a custom icon just for the rings, or you keep it in an extra component and implement the rings as additional elements.
Note that for focus and disabled style, the rings should not be shown, according to Julian.

Comment on lines 106 to 122
<Tooltip
class="staking-feature-tip"
preferredPosition="bottom left"
:container="this">
<div slot="trigger">
<button class="stake" @click="$router.push('/stake')"
@mousedown.prevent
:disabled="(activeCurrency === 'nim' && (!activeAddressInfo || !activeAddressInfo.balance))
|| activeCurrency !== 'nim'"
>
<StakingHeroIcon />
</button>
</div>
<span>
{{ $t('Earn ~304 NIM a month by staking your NIM') }}
</span>
</Tooltip>
Copy link
Member

Choose a reason for hiding this comment

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

The Tooltip should automatically be shown if the user has never clicked the button yet (and has a balance > 0) and not just on hover.
After that, the Tooltip style and content changes, see designs.
Also the rings should only be visible until the user clicked the button for the first time.

},
);
};
onMounted(() => render());
Copy link
Member

Choose a reason for hiding this comment

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

Add:

onUnmounted(() => destroy());

Comment on lines 168 to 170
Vue3ChartJs.install = (app) => {
app.component(Vue3ChartJs.name, Vue3ChartJs);
};
Copy link
Member

Choose a reason for hiding this comment

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

Is this registering the component globally or what is this doing?
If so, I guess you can remove this, as we're always importing all components individually.

Comment on lines 79 to 107
props: {
type: {
type: String,
required: true,
},
data: {
type: Object,
required: true,
},
options: {
type: Object,
default: () => ({}),
},
plugins: {
type: Array,
default: () => [],
},
chartStyles: {
type: Object,
required: false,
default: () => ({
width: '129rem',
height: '35.5rem',
}),
},
},
Copy link
Member

Choose a reason for hiding this comment

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

This component is not reactive to props changes.

@keypress.enter="$event.target.blur()"
:style="`width: ${inputAmountWidth}px;`"
/>
<div class="right-suffix">
Copy link
Member

Choose a reason for hiding this comment

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

ticker would be a class name more in line of how we called that so far.

Comment on lines +138 to +129
const alreadyStakedPercentage = ref(currentPercentage.value);
const alreadyStaked = ref(alreadyStakedAmount.value > 0);
Copy link
Member

Choose a reason for hiding this comment

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

alreadyStaked and alreadyStakedPercentage are not reactive to changes to alreadyStakedAmount and should be computed properties instead.

@sisou sisou force-pushed the staking-draft branch 2 times, most recently from da1b93d to 304a07c Compare September 30, 2021 18:46
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