-
Notifications
You must be signed in to change notification settings - Fork 98
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
Transaction styling #3181
Transaction styling #3181
Conversation
really nice work mario!
|
Thanks! I saw that there is room to improve in the mobile view :-D The transition was on my plans, for sure and will follow the rest of your suggestions. |
@fedekunze i know this PR is close to your heart - you can check it out here https://deploy-preview-3181--lunieio.netlify.com/ |
Codecov Report
@@ Coverage Diff @@
## develop #3181 +/- ##
===========================================
+ Coverage 88.15% 88.83% +0.68%
===========================================
Files 126 125 -1
Lines 1832 1908 +76
Branches 275 291 +16
===========================================
+ Hits 1615 1695 +80
+ Misses 210 206 -4
Partials 7 7
|
Improvement idea: Use icons for the meta information. |
What do you mean? |
}, | ||
computed: { | ||
icon() { | ||
if (this.transactionType === `Sent`) { |
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.
do you know ob the object switch approach?
const dictionary = {
keyA: value,
keyB: value
}
const valueForKey = dictionary[key]
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.
Definitely more cleaner but as I need to make some comparisons for Withdrawal, Vote and Deposit types like:
} else if (this.transactionType.search('Voted') === 0) {
It's possible to do that in a dictionary?
Another option is just to simplify the caption for that messages and include the info in the tx detail, then we can use a dictionary. Maybe a better solution.
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 funky to me. What are the options for transactionType
that there are multiple with Voted
in it?
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.
yeah I think you are on the right track. there should be an enum "transactionType" that should be different then the title.
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 finally implemented that way. The icon name should be the caption string, so that way we don't need a dictionary, only need to handle Update withdraw address
, which is the only specific case. A bit hacky but simplier :-)
computed: {
icon() {
if (this.transactionType.indexOf(` `) !== -1) {
if (this.transactionType === `Update withdraw address`) {
return `/img/icons/activity/Withdrawal.svg`
} else {
return `/img/icons/activity/${this.transactionType.substr(
0,
this.transactionType.indexOf(` `)
)}.svg`
}
} else {
return `/img/icons/activity/${this.transactionType}.svg`
}
}
}
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 am still worried, that the transactionType is the title of the transaction. If we change the title now, we need to change the name of the icon. feels like an unnecessary linkage. But we can improve later.
Hey @jbibla, we have some missing icons for:
|
Also having a generic icon could be useful, just to have a fallback. |
…t mess with nice tx formatting
this is done IMO and should be merged as soon as possible. i think @mariopino and @Bitcoinera encountered a number of issues while working on this code — please make sure you open issues so we can fix these problems in the future! |
Looks great! New format is missing in multisend though, you can check it signing with |
Great seeing Jordan helping out the team here!! |
}, | ||
computed: { | ||
icon() { | ||
if (this.transactionType.indexOf(` `) !== -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 still have concerns around these string searches in comparison to some enum. Also transactionType
is used here as title
which I think is a bad naming.
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.
True! let's do an small refactor (no new features, I promise! ;-)).
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.
@faboweb i think it should be in a new PR!
return transaction.value.amount[0] | ||
} else { | ||
return transaction.value.amount | ||
} | ||
} | ||
|
||
export const getMultiSendCoin = (transaction, sessionAddress) => { |
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.
Misses a comment, why this is necessary
Co-Authored-By: Fabian <frznhope@gmail.com>
</div> | ||
</template> | ||
|
||
<script> | ||
import TransactionIcon from "./TransactionIcon" | ||
import TransactionDetails from "./TransactionDetails" | ||
import { messageType } from "./messageTypes.js" |
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.
here is the messageType enum I would have liked to see in the icon handling as well. no need for a separate form of declaring the types.
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.
But we can do that as a follow up.
Co-Authored-By: Fabian <frznhope@gmail.com>
Closes #3078
Description:
Implement the new transaction page design:
Bonus: Add support for multisend transactions.
Thank you! 🚀
For contributor:
yarn changelog
for a guided process.Files changed
in the github PR explorerFor reviewer: