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.

Sign up for GitHub

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

Jump to bottom

Update transaction details drawer #10374

Merged
merged 1 commit into from May 13, 2024
Merged

Update transaction details drawer #10374

merged 1 commit into from May 13, 2024

Conversation

gustavlrsn
Copy link
Member

@gustavlrsn gustavlrsn commented May 2, 2024 •
edited

Resolve opencollective/opencollective#7396
Project: opencollective/opencollective#7343

Description

Changes to the Transaction details drawer

  • Use internal/legacy ID as the main identifier
  • Add payment method, with updated payment method label of service and type combined.
  • Add accounting category
  • Add merchant ID
  • Update spacing of DataList and create a component out of it (also used in LegalDocumentsDrawer)

Updates the payment method label to be a combination of service and type

Screenshots

image

@vercel Vercel
Copy link

vercel bot commented May 2, 2024 •
edited

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
opencollective-frontend ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 13, 2024 11:54am
opencollective-styleguide ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 13, 2024 11:54am

@gustavlrsn Graphite App
Copy link
Member Author

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @gustavlrsn and the rest of your teammates on Graphite Graphite

@gustavlrsn gustavlrsn force-pushed the 04-30-update_transaction_details_drawer branch from ad159a4 to 3c66bbe Compare May 2, 2024 14:15
@gustavlrsn gustavlrsn mentioned this pull request May 6, 2024
Open
@gustavlrsn gustavlrsn force-pushed the 04-30-update_transaction_details_drawer branch from 3c66bbe to 4f4d7dd Compare May 7, 2024 14:09
@gustavlrsn gustavlrsn force-pushed the 04-30-update_transaction_details_drawer branch from 4f4d7dd to a590089 Compare May 7, 2024 14:53
@gustavlrsn gustavlrsn marked this pull request as ready for review May 8, 2024 10:10
@gustavlrsn gustavlrsn force-pushed the 04-30-update_transaction_details_drawer branch from a590089 to 9fb916d Compare May 8, 2024 10:10
@gustavlrsn gustavlrsn force-pushed the 04-30-update_transaction_details_drawer branch from 9fb916d to 641895c Compare May 8, 2024 10:30
@gustavlrsn gustavlrsn requested a review from kewitz May 8, 2024 10:32
@gustavlrsn gustavlrsn force-pushed the 04-30-update_transaction_details_drawer branch 2 times, most recently from f2f59dc to 995c8f6 Compare May 8, 2024 10:37
@gustavlrsn gustavlrsn force-pushed the 04-30-update_transaction_details_drawer branch from 995c8f6 to fa5c8c2 Compare May 8, 2024 13:43
kewitz
kewitz approved these changes May 9, 2024
View reviewed changes
Copy link
Contributor

@kewitz kewitz left a comment

Choose a reason for hiding this comment

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

Looking and feeling good. Nice work!

components/dashboard/sections/transactions/TransactionsTable.tsx
onClickRow={(row, menuRef) => openDrawer(row.id, menuRef)}
onClickRow={(row, menuRef) => {
openDrawer(row.id, menuRef);
}}
onHoverRow={row => setHoveredGroup(row?.original?.group ?? null)}
Copy link
Contributor

Choose a reason for hiding this comment

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

I caught something while testing the performance of the Transactions dashboard: onHoverRow causes the whole table to re-render when hovering over a different group. This is fine for now because we do not display a lot of transactions in the table right now, but it could become an issue if we decide to increase the number of rows being displayed at the same time. A possible solution would be implementing this effect using CSS selectors and data properties.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point, created an issue to track this: opencollective/opencollective#7405

components/CopyId.tsx Outdated
export const CopyID = ({ children, tooltipLabel = <FormattedMessage defaultMessage="Copy ID" id="wtLjP6" /> }) => {
export const CopyID = ({
children,
prepend = '',
Copy link
Contributor

Choose a reason for hiding this comment

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

It would probably be more straightforward to inform a value property you want to be copied and always display children as the label.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good suggestion, pushed a fix for this!

@gustavlrsn gustavlrsn force-pushed the 04-30-update_transaction_details_drawer branch from fa5c8c2 to edcfb06 Compare May 13, 2024 10:50
@gustavlrsn gustavlrsn mentioned this pull request May 13, 2024
Open
@gustavlrsn gustavlrsn force-pushed the 04-30-update_transaction_details_drawer branch from edcfb06 to 176f3a4 Compare May 13, 2024 10:56
@gustavlrsn gustavlrsn force-pushed the 04-30-update_transaction_details_drawer branch from 176f3a4 to 456ba44 Compare May 13, 2024 11:45
@gustavlrsn gustavlrsn merged commit 81a1575 into main May 13, 2024
18 of 19 checks passed
@gustavlrsn gustavlrsn deleted the 04-30-update_transaction_details_drawer branch May 13, 2024 12:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kewitz kewitz kewitz approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

UI: Update Transaction details drawer
2 participants
@gustavlrsn @kewitz