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

Fix: Activity table fixes and table refactoring #3757

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

mmioana
Copy link
Contributor

@mmioana mmioana commented Nov 22, 2024

Description

This PR aims to refactor (a tiny bit) the Table component and addresses the following issues

  • The pagination should be displayed inside the table container and not outside for all tables
    Figma link
  • When there are not enough items for there to be more than 1 page, the recent activity table should go as tall as it can until it either fills the bottom of the dashboard area, or reaches the max height it would be with 7 items.
  • For activity tables, the content - namely the metadata and status pill - is moving upon expanding a row

Tabs closed (note the position of the pill, title and metadata):
image
With one tab open (again, note the position of the pill, title and metadata):
image

  • For the recent activity table, the status pills should have only 16px between the expandable icon
image

Testing

TODO: Please test the above mentioned issues have been resolved, as well as the Table component refactoring didn't introduce bugs.

Screen.Recording.2024-11-22.at.16.01.32.mov
  • Step 4. Ensure the description and status pills of the other actions doesn't change their position
  • Step 5. Check the spacing between the status pill and expandable icon is 16px

Screenshot 2024-11-22 at 16 03 44

  • Step 6. Now go to http://localhost:9091/planex/activity

  • Step 7. Check the table pagination is displayed inside the table
    Screenshot 2024-11-22 at 16 05 45

  • Step 8. Run node scripts/create-colony-url.js and complete creating a colony

  • Step 9. Create a Mint tokens action
    Screenshot 2024-11-22 at 16 07 19

  • Step 10. Go to the newly created colony Dashboard page

  • Step 11. Check the height of the Recent activity table - it should occupy the whole available page height
    Screenshot 2024-11-22 at 16 09 01

Further testing

Please check the tables from
http://localhost:9091/planex/balances
http://localhost:9091/planex/incoming
http://localhost:9091/account/crypto-to-fiat

Also please try creating an Advanced payment or Manage tokens action and check the tables there for both mobile and desktop

Any further testing ideas are highly welcomed 🙌 and thanks a lot for reaching this far in the testing 🥇

Diffs

Changes 🏗

  • The Table component has been split up into more manageable partial components

Resolves #2695
Resolves #3550
Resolves #3668

Refactor: Table component
Fix: Add flex for activity table status column
Fix: Styling updates to ensure min height for recent activity table
@mmioana mmioana self-assigned this Nov 22, 2024
@mmioana mmioana marked this pull request as ready for review November 22, 2024 15:13
@mmioana mmioana requested a review from a team as a code owner November 22, 2024 15:13
rdig
rdig previously approved these changes Nov 25, 2024
Copy link
Member

@rdig rdig left a comment

Choose a reason for hiding this comment

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

Very nice, all 3 cases I tested, mobile, desktop, and single action in the list, work as described.

Nicely done!

Screencast.from.2024-11-25.21-30-59.mp4
Screencast.from.2024-11-25.21-31-41.mp4
Screencast.from.2024-11-25.21-33-09.mp4

Copy link
Contributor

@iamsamgibbs iamsamgibbs left a comment

Choose a reason for hiding this comment

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

Great job on this @mmioana These tables are a pain to deal with so nice job handling them here!

The only issue I noticed is here on the Crypto to fiat page:

Screenshot 2024-11-26 at 10 14 45

Otherwise everything else is great.

Expandable rows without pills shifting:

Screen.Recording.2024-11-26.at.10.02.29.mov

All activity:

Screenshot 2024-11-26 at 10 11 06

Empty activity table goes up to max height:
Screenshot 2024-11-26 at 10 12 06
Screenshot 2024-11-26 at 10 12 26

And with 1 activity:
Screenshot 2024-11-26 at 10 13 08
Screenshot 2024-11-26 at 10 13 34

Screenshot 2024-11-26 at 10 13 47

Other tables are unbroken:
Screenshot 2024-11-26 at 10 14 21
Screenshot 2024-11-26 at 10 14 34
Screenshot 2024-11-26 at 10 29 40

Fix: Update FiatTransfersTable paddings
@mmioana
Copy link
Contributor Author

mmioana commented Nov 26, 2024

Hey @rdig @iamsamgibbs thanks for your reviews 🙏
I pushed a fix for the Crypto to fiat page's table, if you can please re-check 🙌

iamsamgibbs
iamsamgibbs previously approved these changes Nov 27, 2024
Copy link
Contributor

@iamsamgibbs iamsamgibbs left a comment

Choose a reason for hiding this comment

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

Thanks for the change, looks perfect now!

Screenshot 2024-11-27 at 10 25 36 Screenshot 2024-11-27 at 10 25 43

bassgeta
bassgeta previously approved these changes Nov 27, 2024
Copy link
Contributor

@bassgeta bassgeta left a comment

Choose a reason for hiding this comment

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

Definitely a step in the right direction with the table, it was about time we took everythin out into separate components.
Love that we have 2 different layouts now, it was quite confusing before :')
Overall, super happy with the foundations laid in this PR, let's hope we can get to refactoring that beast into something manageable in the future 🤞

  1. Expanding an existing action ✔️
    image
  2. Padding is 16px ✔️
    image
  3. No pagination on latest activity when only 1 action ✔️
    image
  4. Tallboi recent activity table ✔️
    image

Copy link
Contributor

@rumzledz rumzledz left a comment

Choose a reason for hiding this comment

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

Almost at the finish line @mmioana ! All the test cases you mentioned are looking good on my end but I just noticed one wee problem. The error state for the table seems to be slightly off:

Manage permissions form

Master Your branch
Screenshot 2024-11-28 at 17 11 14 Screenshot 2024-11-28 at 17 10 51

Advanced payments form

Master Your branch
Screenshot 2024-11-28 at 17 12 00 Screenshot 2024-11-28 at 17 09 21

Fix: Tables error state after review
@mmioana mmioana dismissed stale reviews from bassgeta and iamsamgibbs via 0a20ef4 November 28, 2024 12:01
@mmioana
Copy link
Contributor Author

mmioana commented Nov 28, 2024

Thanks so much for finding this issue @rumzledz 🙌

I pushed a fix, so could you guys @iamsamgibbs @bassgeta @rumzledz please re-check? 🙏

@mmioana mmioana marked this pull request as draft November 28, 2024 15:35
Fix: Remove tokens table border
@mmioana
Copy link
Contributor Author

mmioana commented Nov 28, 2024

@rumzledz found an issue with duplicated borders for table shown on the http://localhost:9091/planex/incoming page when expanding a token, but now it should be fixed

Screenshot 2024-11-28 at 17 06 38

@mmioana mmioana marked this pull request as ready for review November 28, 2024 16:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants