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.

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

Dashboard: TransactionHistory component #407

Merged
merged 40 commits into from
May 29, 2024

Conversation

kpyszkowski
Copy link
Contributor

@kpyszkowski kpyszkowski commented May 8, 2024

Closes: #366

Goal:

To implement TransactionHistory component

Features:

  • generic Pagination component with direction-aware animated page transition
  • TransactionHistory empty state

Pagination component anatomy:

<Pagination data={[]}>
  <PaginationPage>
    {(pageData) => /* render page items */}
  </PaginationPage>
  <PaginationButton mode={'previous' | 'next'} />
  <PaginationStatus dataLabel={string} />
</Pagination>

TransactionHistory component anatomy:

<TransactionHistory data={[
  {
    date: string
    type: string
    amount: string
    address: string
    url: string
  },
  // ...
]} />

Designs:

localhost_5173_ (2)

Notes:

To prevent visual artifacts when page is transitioning it's mandatory to set overflow: hidden CSS property on container element. It's impossible to embed it into component.

Known issues:

  • Pagination component's pageData type assertion is not working, dunno how to do it 🤷🏼‍♂️ - it's definitely doable so I'd appreciate any suggestion
  • The Pagination container is not resizing smoothly when page size changes (visible when switching between last and one-before-last pages)
  • TransactionHistory empty state illustration is not SVG

Testing:

dashboard-transaction-history.patch

Copy link

netlify bot commented May 8, 2024

Deploy Preview for acre-dapp-testnet ready!

Name Link
🔨 Latest commit c56be9f
🔍 Latest deploy log https://app.netlify.com/sites/acre-dapp-testnet/deploys/6656f5d2637e760008e68e68
😎 Deploy Preview https://deploy-preview-407--acre-dapp-testnet.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Update exports from module to default
Fixed content shift adding height animation of `PaginationPage` with use
of `useSize` hook.
Changed `currentPage` to `page`
@kpyszkowski kpyszkowski force-pushed the dashboard/transaction-history branch from cd35f67 to 577599c Compare May 8, 2024 22:25
Adjusted transition, implement customizable page spacing with support of
Chakra tokens, refactored variants
Adjusted component to pass props to container
pageSpacing?: number | string
}

function PaginationPage(props: PaginationPageProps) {
Copy link
Contributor

Choose a reason for hiding this comment

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

What do you think about changing the name to AnimatedPaginationPage? This component is not obligatory when using PaginationContext.Provider. Therefore, I would suggest giving a more descriptive name.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

By saying 'Animated' we indicate there might be not animated variant which is not true.
Animation is not a toggleable feature but integral part of the implementation.
Redundant.

Copy link
Contributor

Choose a reason for hiding this comment

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

By saying 'Animated' we indicate there might be not animated variant which is not true.
Animation is not a toggleable feature but integral part of the implementation.
Redundant.

Note that component Pagination is actually a provider. As I mentioned in my previous comments (#407 (comment), #407 (comment)) I think we should separate it on styles and give it a more descriptive name for what it is responsible for.

So I don't need to use PaginationPage at all to render my content. I can define this content on my own and I will not get any error. I can create a page for pagination without animation. The name PaginationPage doesn't quite describe what we can use it for. Therefore, I suggest that the name should refer to the animation so that it is clear what the component does.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But you need. That's the only way to access pageData.

The name PaginationPage doesn't quite describe what we can use it for.

So what does "Animated" changes? It says it's animated. That's all.

I suggest that the name should refer to the animation so that it is clear what the component does.

But animation is not a purpose... the purpose is to split data collection in to pages and allow to display and control it. Animation is just a feature. In mature UI kits every component is animated and nobody names it "Animated".

Copy link
Contributor

Choose a reason for hiding this comment

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

I will answer in one thread to avoid making a mess.

But you need. That's the only way to access pageData.

It is not true. We can use the Pagination component (which has PaginationContext.Provider inside) and then pass the component as children that use the usePagination. The same way PaginationPage does it. So I do not have to use this component to create pagination.

Probably using the "Animated" prefix here will not solve this problem. But It is not quite clear to me how to use these components. Because as I understand it, we treat Pagination and PaginationPage as a container. However, when we use PaginationButton and PaginationStatus, we need to create a container for it. So, from one side we provide some freedom but on the other side some of the styles (basic styles like gap, flex-direction) are in the Pagination or PaginationPage component. That's what's not quite clear to me.

I have no problem with leaving it like that. But I have a feeling that we could somehow improve this part. That's why I also suggested maybe separating PaginationContext.Provider from styles.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So I do not have to use this component to create pagination.

That's why I wouldn't like to expose the context itself by moving the context to shared contexts directory. It's not meant to be used standalone. Same about the hook. It's just a part of implementation of the Pagination component. And the only way to consume this context should be by rendering Pagination component.

So, from one side we provide some freedom but on the other side some of the styles (basic styles like gap, flex-direction) are in the Pagination or PaginationPage component. That's what's not quite clear to me.

Styles of PaginationButton and PaginationStatus can be customized via props.
The PaginationPage is not styled in any way. It just exposes a chunk of data as a FaaC and allows to render it (and style) freely in a desired way.

The main goal to create a generic composable and completely reusable component - you want pagination, use Pagination component, structure, compose and style it freely.

Currently the only improvements I see for the future currently are:

  • Make the PaginationButton headless so I won't have any styles embedded
  • Fix the types so the pageData type in PaginationPage can be inferred automatically

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Generally speaking - from what I understand - each context provider defined in contexts directory is meant to be reusable as standalone context. You might assume I'd like to define PaginationContext as standalone reusable entity the same way but it wasn't my intention.
I placed it alongside with components definitions (and didn't reexport it) to hide it and therefore limit the use of this context only for the internal purpose of the Pagination component (and all its children).

Let's not block the PR with this discussion. We can come back to it later.

Copy link
Contributor

Choose a reason for hiding this comment

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

It is not a blocker and we can always improve it later. Just flagging that the structure of this component and the way it is used is not quite clear to me.

@kpyszkowski kpyszkowski requested a review from kkosiorowska May 14, 2024 14:17
pageSpacing?: number | string
}

function PaginationPage(props: PaginationPageProps) {
Copy link
Contributor

Choose a reason for hiding this comment

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

By saying 'Animated' we indicate there might be not animated variant which is not true.
Animation is not a toggleable feature but integral part of the implementation.
Redundant.

Note that component Pagination is actually a provider. As I mentioned in my previous comments (#407 (comment), #407 (comment)) I think we should separate it on styles and give it a more descriptive name for what it is responsible for.

So I don't need to use PaginationPage at all to render my content. I can define this content on my own and I will not get any error. I can create a page for pagination without animation. The name PaginationPage doesn't quite describe what we can use it for. Therefore, I suggest that the name should refer to the animation so that it is clear what the component does.

@kkosiorowska kkosiorowska force-pushed the dashboard/transaction-history branch from bcc029c to b0b0583 Compare May 16, 2024 09:21
Copy link
Contributor

@kkosiorowska kkosiorowska left a comment

Choose a reason for hiding this comment

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

We are so close to do a merge. 🚀 Let's remove unnecessary things and improve the styles a bit. The columns overlap but we have some space to properly display the text.

Screenshot 2024-05-15 at 13 24 32

In my case, the animation of switching pagination pages looks strange. Could we improve it a bit?

Screen.Recording.2024-05-16.at.08.01.08.mov

Copy link
Contributor

@kkosiorowska kkosiorowska left a comment

Choose a reason for hiding this comment

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

Almost ready 🚢 Just a few small comments. The dapp-format action failed so let's fix it. Additionally, we have removed the ActivitiesList component by mistake. After these changes, we will be ready to merge.

Copy link
Contributor

@kkosiorowska kkosiorowska left a comment

Choose a reason for hiding this comment

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

🚀🚀🚀

@kkosiorowska kkosiorowska merged commit c699247 into main May 29, 2024
24 checks passed
@kkosiorowska kkosiorowska deleted the dashboard/transaction-history branch May 29, 2024 09:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

dApp Dashboard: Transaction History - Deposit
3 participants