-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
feat: transaction thumbnail component #3301
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
3 Skipped Deployments
|
|
@@ -2,7 +2,7 @@ import { html, LitElement } from 'lit' | |||
import { customElement } from '../../utils/WebComponentsUtil.js' | |||
import { resetStyles } from '../../utils/ThemeUtil.js' | |||
import '../../components/wui-text/index.js' | |||
import '../wui-transaction-visual/index.js' | |||
import '../wui-transaction-thumbnail/index.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.
Imported it like this so it doesn't throw ts error
export const transactionThumbnailOptions: TransactionThumbnailType[] = [ | ||
'token', | ||
'nft', | ||
'swap', | ||
'fiat', | ||
'unknown' | ||
] |
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.
The previous values are coming from the possible values of Zerion API, so we might want to keep the same array because we don't control this value manually on parent component
@@ -245,6 +248,10 @@ export type ButtonVariant = | |||
export type ButtonShortcutVariant = 'accent' | 'secondary' | |||
export type ButtonLinkVariant = 'accent' | 'secondary' | |||
|
|||
export type TransactionThumbnailType = TransactionType | 'fiat' | 'unknown' | 'nft' |
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 included both TransactionType
and 'fiat' | 'unknown' | 'nft'
in case we need it
export const spacing = { | ||
'0.5': '2px', | ||
'01': '2px', |
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.
Why not 0.5
?
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.
CSS variables cannot have decimals, so something like --apkt-spacing-0.5
will break
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.
@magiziz Can we add tests and merge?
I'll mark this PR as draft since we need to re-design this component a bit. |
Coverage Report
File CoverageNo changed files found. |
Description
Added transaction thumbnail component
Type of change
Associated Issues
For Linear issues: Closes APKT-1510
Showcase (Optional)
Checklist