-
Notifications
You must be signed in to change notification settings - Fork 17
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
Improvements to the header #594
Improvements to the header #594
Conversation
Codecov Report
@@ Coverage Diff @@
## master #594 +/- ##
==========================================
+ Coverage 95.77% 95.79% +0.01%
==========================================
Files 88 88
Lines 2203 2210 +7
Branches 305 307 +2
==========================================
+ Hits 2110 2117 +7
Misses 51 51
Partials 42 42
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks pretty good to me. 👍
Just minor things and some questions. 😃
matTooltip="{{ | ||
tokenBalancesOpen | ||
? '' | ||
: 'Show all on-chain balances' | ||
}}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick: Sorry I forgot again if this was easily possible in Angular. But this really bloats the template with details. If possible it would be cool if this could be defined as some kind of computed data and you just have something like matTooltip="{{ showBalancesTooltip }}"
. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's not a feature like the computed data of Vue. You can use a function but that would get invoked for every change detection. But I just found out that there is matTooltipDisabled
property you can use to not show the tooltip conditionally. That makes more sense in this case. Thank you for helping me finding this!
text-transform: uppercase; | ||
color: $light-grey; | ||
|
||
&--eth { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick: modifiers of the BEM scheme are usually behavior descriptive adjectives. So eventually --upper-case
or similar fits better. This would also make it re-usable in case you would need it for anything else that needs the same modification. But this is really a stupid nitpick. 😆
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are totally right :)
[token]="token" | ||
></app-balance-with-symbol> | ||
<button | ||
*ngIf="(network$ | async)?.chainId !== 1" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would consider to give this expression a helpful name that reads better. E.g. ngIf="!connectedToMainnet"
What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, makes sense as it also was like this before :)
The mint button is now displayed next to the on-chain token balance because it doesn't increase the off-chain balance.
9ded2d7
to
4f0d9d5
Compare
4f0d9d5
to
b6dc02b
Compare
b6dc02b
to
5041dd9
Compare
Fixes #557
This PR prepares the header for adding the UDC deposit and withdraw functionality (#556) according to Sash's updated design.
We moved the mint button next to the on-chain balance. Before it was next to the off-chain balance even though it doesn't affect it.
Having the selected token first in the header is necessary for being able to still mint all tokens.