Conversation
|
|
@jbrauck-unchained is attempting to deploy a commit to the Unchained Team on Vercel. A member of the Team first needs to authorize it. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
…ed/caravan into transaction-flow trying to fix this
@jbrauck-unchained maybe you'll have do the formatting ( run the linter ) and get the unsed imports out for the ci to pass :)
|
|
One small nit: maybe we can add ~ 0.00% total so we can tell the user we are rounding up here and they don't think of this as a bug |
maybe we can remove one of the |
|
This pull request has been inactive for 30 days and has been marked as stale. It will be closed in 7 days if no further activity occurs. To keep this PR open, add the "long-lived" label or comment on it. |
Legend101Zz
left a comment
Legend101Zz
left a comment
There was a problem hiding this comment.
@jbrauck-unchained thanks for the awesome work , I tested it and it looks super good to me , now I guess we can move towards the code changes , the main concern I have is over modularity and an IIFE used in rendering of the flow status
apps/coordinator/src/components/Wallet/TransactionFlowDiagram.tsx
Outdated
Show resolved
Hide resolved
apps/coordinator/src/components/Wallet/TransactionFlowDiagram.tsx
Outdated
Show resolved
Hide resolved
| return `${address.slice(0, 10)}...${address.slice(-8)}`; | ||
| }; | ||
|
|
||
| const getStatusDisplay = () => { |
There was a problem hiding this comment.
this can go in the utils file then
| }; | ||
|
|
||
| // Format script type for display | ||
| const formatScriptType = (scriptType?: string) => { |
There was a problem hiding this comment.
can you add this and formatAddress in the general utils file as they can be used by other components too :)
There was a problem hiding this comment.
Is there a different general utils file you are talking about? For now I added to the TransactionFlowUtils.ts file
There was a problem hiding this comment.
that works perfectly for me :)
- Create TransactionFlowDiagram directory structure with separate files: - index.tsx: Main component file (cleaner, more focused) - hooks.ts: Custom useFlowPaths hook for SVG path calculations - utils.ts: Utility functions (buildCurvePath, formatAddress, formatScriptType, getScriptTypeColor, getStatusDisplay) - FlowDrawers.tsx: Input and output drawer components - FlowSummary.tsx: Summary section with legend and transaction totals - Move formatSats and formatAddress to transactionFlowUtils.ts for reuse - Remove old 1704-line monolithic TransactionFlowDiagram.tsx file This addresses code review feedback to improve maintainability and readability.
- Remove unused variables in TransactionPreview.jsx render method - Fix prettier formatting in TransactionsTable.tsx - Fix prettier formatting in useTransactionDetails.ts - Remove unused blockHeight variable in transactionFlowUtils.ts
Legend101Zz
left a comment
Legend101Zz
left a comment
There was a problem hiding this comment.
Thanks alot for the changes , it's looking really good now , nothing from my end now :)
@bucko13 can you please review it when you get the chance ...
3 participants
What kind of change does this PR introduce?
This introduces a new tranasction flow diagram feature
Issue Number:
Fixes #393 #393
Snapshots/Videos:
If relevant, did you update the documentation?
Summary
I like what sparrow does to try and give a user a sense of understanding their own transaction. I think this is a small lift to gain feature parity in a place that makes caravan more educational.
Does this PR introduce a breaking change?
Checklist
npm run changeset)Other information
Have you read the contributing guide?
For information on creating and using changesets, please refer to our documentation on changesets.