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

Feat/implement design reporting tab #1384

Closed
wants to merge 30 commits into from
Closed

Feat/implement design reporting tab #1384

wants to merge 30 commits into from

Conversation

Hornebom
Copy link
Contributor

@Hornebom Hornebom commented May 16, 2023 •
edited
Loading

Description

Note
Working pull request with verified commits: #1520
This pr is only for reference

Issue: #1199

Approvals

  • Dev
  • Product

@Hornebom
Refactor Reporting tab
6bb18e6 
- refactor to use context
- split up into sub components
@Hornebom
Design reporting
9697988 
-extends TableGroup with `rounded` prop -> as required by design
- extends InputBox and Select component with `bordered` variation
- set csv file names
- refactor markup ReportCompnent
- creates new DateRange fabric component
@Hornebom
Merge branch 'main' into feat/implement-design-reporting-tab
3b0363c
@Hornebom
Merge branch 'main' into feat/implement-design-reporting-tab
6700b5d
@Hornebom
update to latest loan changes
8f61b26 
wip state
@Hornebom
Reporting tab
a1dbace
@github-actions
Copy link

github-actions bot commented May 16, 2023 •
edited
Loading

PR deployed in Google Cloud
URL: https://app-pr1384.k-f.dev
Commit #: a8ae436
To access the functions directly check the corresponding deploy Action

@annamehr
Copy link
Contributor

Some feedback:

Pool balance:

  • should we not display group by month if date range is last week and instead only display day in the dropdown?
  • should we display the values on the left side instead of on the right side and have this big white spacing in between?

Screenshot 2023-05-17 at 12 53 00

@hieronx
Copy link
Contributor

hieronx commented May 19, 2023

A few testing notes:

  • I wonder if it would make sense to fix the row names (the first column of the data table) and just scroll the remaining columns. If you look on https://app-pr1384.k-f.dev/pools/3317493590/reporting and scroll right, the row names (Pool value, Asset value, ...) quickly disappear.
  • The data table width seems to be limited to the initial screen width and not grow with the full scrollable width.
Screenshot 2023-05-19 at 12 06 17
  • I think we should parse/re-format the borrower and investor tx types a bit.

For the borrower tx type, I would suggest to just capitalize the first letter. E.g. CREATED => Created, REPAID => Repaid. And change BORROWED to Financed (as we consistently use this naming in the UI, while borrowing is just a term used in the code).

For the investor tx type, I would suggest:
INVEST_ORDER_UPDATE => Investment order updated
REDEEM_ORDER_UPDATE => Redemption order updated
INVEST_ORDER_UPDATE if currency amount is 0 => Investment order cancelled
REDEEM_ORDER_UPDATE if currency amount is 0 => Redemption order cancelled
INVEST_EXECUTION => Investment executed
REDEEM_EXECUTION => Redemption executed
INVEST_COLLECT => [TRANCHE_TOKEN_SYMBOL] received in wallet (e.g. NS2DRP received in wallet)
REDEEM_COLLECT => [POOL_CURRENCY_SYMBOL] received in wallet (e.g. USDT received in wallet)
TRANSFER_IN => Deposited [TRANCHE_TOKEN_SYMBOL]
TRANSFER_OUT => Withdrawn [TRANCHE_TOKEN_SYMBOL]

  • The Loan column of the Borrower transactions view should be renamed to Asset ID, and should just display the number, not the pool ID prefix and the -. Similarly, the epoch column data should just be the epoch # (1, 2, 3, ...), not include the pool id prefix.
  • The Token amount column of the Borrower transactions view should be [POOL_CURRENCY_SYMBOL] amount (e.g. USDT amount)

@gpmayorga gpmayorga closed this Jun 5, 2023
@gpmayorga gpmayorga reopened this Jun 5, 2023
@gpmayorga
Copy link
Collaborator

gpmayorga commented Jun 5, 2023 •
edited
Loading

For some reason this PR does not trigger Github Actions correctly 🤷‍♂️
Opening and closing this PR should trigger the following:
https://github.com/centrifuge/apps/blob/feat/implement-design-reporting-tab/.github/workflows/prepare-pr.yml

@Hornebom
Merge branch 'main' into feat/implement-design-reporting-tab
f2580ea
@Hornebom
formatting table data to human readable strings
55c0b77
@Hornebom
adds currency symbol in front of amount
8706689
@Hornebom
visual display reporting tables
20f3a2d 
- extends Spinner to accepts ShelfProps -> enable setting margin directly on Spinner
- set minWidth on DataTable via new useElementScrollSize -> nested flex container need specific width otherwise it will inherit it's parent container width (borders and background hover color won't span on whole table width)
- refactor loading state of reports
- adds custom column sizes for each report table
@Hornebom
Merge branch 'main' into feat/implement-design-reporting-tab
8db8bc5
@Hornebom
removes developement component rendering
3a96551
@Hornebom
Merge branch 'main' into feat/implement-design-reporting-tab
7de8431
@Hornebom
manually solve merge conflict
692a308 
bring changes (#1476) from Report to report > index
@Hornebom Hornebom closed this Jul 4, 2023
@Hornebom Hornebom reopened this Jul 4, 2023
@annamehr
Copy link
Contributor

My testing feedback.. it already looks a lot better! @Hornebom

On the first screenshot:

  • It looks like the reporting data for the US treasuries pool is not synced and therefore not displayed. The reserve of BT test pool is 70k USDT but displayed is 0.0 USDT and Value locked is 1,006,888 USDT and displayed is only 0.0 USDT.
Screenshot 2023-07-10 at 12 15 30

Second screenshot:

  • I want to see the pool balance of every day last week but I can only see July 6th, looks like the group by filter isn't working yet.
Screenshot 2023-07-10 at 12 17 39

onnovisser
onnovisser reviewed Aug 1, 2023
View reviewed changes
fabric/src/components/DateRange/index.tsx Outdated
const [value, setValue] = React.useState(defaultOption.value)
const [startDate, setStartDate] = React.useState(start || new Date())

React.useEffect(() => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

this can just be in the state initializer

const [startDate, setStartDate] = React.useState(getDate[defaultOption.value](end))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

refactored

onnovisser
onnovisser reviewed Aug 1, 2023
View reviewed changes
centrifuge-app/src/components/DataTable.tsx Outdated
@@ -68,6 +69,7 @@ export const DataTable = <T extends Record<string, any>>({
)

const [currentSortKey, setCurrentSortKey] = React.useState(defaultSortKey || '')
const [setNode, { scrollWidth }] = useElementScrollSize()
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think a better API for this would be

const ref = useRef()
const { scrollWidth } = useElementScrollSize(ref)

doing a setState for setting the ref seems like a bad idea

Copy link
Contributor Author

Choose a reason for hiding this comment

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

refactored

@Hornebom Hornebom mentioned this pull request Aug 4, 2023
Merged
1 task
@Hornebom Hornebom closed this Aug 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@onnovisser onnovisser onnovisser approved these changes

@hieronx hieronx Awaiting requested review from hieronx

@annamehr annamehr Awaiting requested review from annamehr

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

Successfully merging this pull request may close these issues.
5 participants
@Hornebom @annamehr @hieronx @gpmayorga @onnovisser