feat(suite): change assets table #14625

enjojoy · 2024-09-30T12:05:52Z

Description

Changed the cards grid to the new Table component

Related Issue

Resolve #13152

Screenshots:

packages/suite/src/views/dashboard/AssetsView/AssetTable/AssetTableHeader.tsx

tomasklim

The table should behave the same as before 😢

Before:

Screen.Recording.2024-09-30.at.19.48.41.mov

Now:

Screen.Recording.2024-09-30.at.19.49.23.mov

scrolling is broken

Screen.Recording.2024-09-30.at.19.45.55.mov

adamhavel · 2024-10-01T08:58:48Z

The table should behave the same as before 😢

Before:

Screen.Recording.2024-09-30.at.19.48.41.mov

Now:

Screen.Recording.2024-09-30.at.19.49.23.mov

scrolling is broken

Screen.Recording.2024-09-30.at.19.45.55.mov

The change in color on row hover is something we would have to add to the Table component. Not sure if by default, or activated by a prop?

tomasklim · 2024-10-01T10:25:43Z

The table should behave the same as before 😢
Before:
Screen.Recording.2024-09-30.at.19.48.41.mov
Now:
Screen.Recording.2024-09-30.at.19.49.23.mov
scrolling is broken
Screen.Recording.2024-09-30.at.19.45.55.mov

The change in color on row hover is something we would have to add to the Table component. Not sure if by default, or activated by a prop?

Maybe activate by prop? But I like it, I would add it also for tokens table

enjojoy · 2024-10-01T10:40:08Z

The table should behave the same as before 😢
Before:
Screen.Recording.2024-09-30.at.19.48.41.mov
Now:
Screen.Recording.2024-09-30.at.19.49.23.mov
scrolling is broken
Screen.Recording.2024-09-30.at.19.45.55.mov

The change in color on row hover is something we would have to add to the Table component. Not sure if by default, or activated by a prop?

Maybe activate by prop? But I like it, I would add it also for tokens table

Should I then add it as additional styling for now or should I add the prop functionality/wait for it to be added?

adamhavel · 2024-10-01T15:40:58Z

The table should behave the same as before 😢

Before:

Screen.Recording.2024-09-30.at.19.48.41.mov

Now:

Screen.Recording.2024-09-30.at.19.49.23.mov

scrolling is broken

Screen.Recording.2024-09-30.at.19.45.55.mov

The change in color on row hover is something we would have to add to the Table component. Not sure if by default, or activated by a prop?

Maybe activate by prop? But I like it, I would add it also for tokens table

Should I then add it as additional styling for now or should I add the prop functionality/wait for it to be added?

Feel free to add it yourself. Something like highlightRows? As it will be a Table prop, you'd have to create a context so that the Row can reach it, though.

enjojoy · 2024-10-01T15:47:57Z

The table should behave the same as before 😢

Before:

Screen.Recording.2024-09-30.at.19.48.41.mov

Now:

Screen.Recording.2024-09-30.at.19.49.23.mov

scrolling is broken

Screen.Recording.2024-09-30.at.19.45.55.mov

The change in color on row hover is something we would have to add to the Table component. Not sure if by default, or activated by a prop?

Maybe activate by prop? But I like it, I would add it also for tokens table

Should I then add it as additional styling for now or should I add the prop functionality/wait for it to be added?

Feel free to add it yourself. Something like highlightRows? As it will be a Table prop, you'd have to create a context so that the Row can reach it, though.

I will add it myself, yes, by the way on develop hover didn't work on light theme, I will try to fix that as well

adamhavel · 2024-10-01T18:41:31Z

packages/components/src/components/Table/HighlightContext.tsx

@@ -0,0 +1,25 @@
+import React, { createContext, useContext } from 'react';


I think it's not necessary to have a separate file. I'd do it the same way it's done in TableHeader for such a simple context.

adamhavel · 2024-10-01T18:42:11Z

packages/components/src/components/Table/Table.tsx

-                backgroundColor={mapElevationToBackgroundToken({ $elevation: parentElevation })}
-            />
-        </ShadowContainer>
+        <HighlightProvider highlightRowOnHover={hightlightRowOnHover}>


I'd just name it TableContext.Provider and useTable

adamhavel · 2024-10-01T18:44:11Z

packages/suite/src/views/dashboard/AssetsView/AssetTable/AssetTableHeader.tsx

-        <Header /> {/* Logo */}
-        <Header>
+    <Table.Header>
+        <Table.Cell colSpan={3}>


This is missing Table.Row. You probably deleted it because you don't want it to be highlighted? You can use the existing context hook from TableHeader to get info if the row is in header or not.

Yes, I didn't want it to be affected by the styling, okay, thank you, I will use the hook

tomasklim

I think button and arrow should change color on row hover because on hover of buttons itself, the background is not visible

Same in tokens table

On click of row the same action is done as on on click of arrow button, maybe it should become active?

There is cursor pointer on tokens table but you cannot click it so the cursor pointer should not be there
So you should probably set it only if onClick is available

Zero balance row does not occupy whole width so it does not change color of whole row. I recommend adding empty <td></td>s

packages/components/src/components/Table/TableCell.tsx

packages/suite/src/views/dashboard/AssetsView/AssetTable/AssetTableHeader.tsx

packages/suite/src/views/dashboard/AssetsView/AssetTable/AssetRow.tsx

enjojoy · 2024-10-03T13:01:41Z

Zero balance row does not occupy whole width so it does not change color of whole row. I recommend adding empty <td></td>s

This is fixed, zero balance row occupies the whole row, also the hover and cursor pointer is applied only to the rows that have an onClick property.

enjojoy · 2024-10-03T13:02:21Z

There is cursor pointer on tokens table but you cannot click it so the cursor pointer should not be there
So you should probably set it only if onClick is available

Done✅

enjojoy · 2024-10-03T13:26:23Z

On click of row the same action is done as on on click of arrow button, maybe it should become active?

AFAIK buttons have only 2 states: normal one and the change of the appearance on hover, but not on click, do you mean that the button will behave the same as the row, which means it will be hovered when the row is hovered?

Or do you mean that it should have an entirely new state when clicked, the "active" state.

If you mean that it should react to the onClick, I would approach it generically and make all the buttons reactive to the onClick, then we could use it here as well instead of making just one button behave differently.

As discussed before, this button has a purely "hinting" purpose, for the user to understand that it's possible to click on the row.

enjojoy · 2024-10-03T13:29:13Z

I think button and arrow should change color on row hover because on hover of buttons itself, the background is not visible

As discussed before, I'm creating an issue for this. This problem occurs because elevation colors are repeating themselves.

enjojoy · 2024-10-03T13:39:06Z

I think button and arrow should change color on row hover because on hover of buttons itself, the background is not visible

As discussed before, I'm creating an issue for this. This problem occurs because elevation colors are repeating themselves.

#14711

adamhavel · 2024-10-03T14:29:27Z

packages/components/src/components/Table/TableRow.tsx

+                })};
+
+                & > td:first-child {
+                    background: linear-gradient(


Perhaps make a const or func out of this so you can use it here and in the Cell? I'll try to figure out if there's a better way to do this, but I think it's good enough for now:)

tomasklim

Last small nits and lets merge it

tomasklim · 2024-10-04T10:00:55Z

packages/suite/src/views/dashboard/AssetsView/AssetTable/AssetRow.tsx

@@ -221,28 +117,28 @@ export const AssetRowSkeleton = (props: { animate?: boolean }) => {
    const animate = props.animate ?? shouldAnimate;


Please move AssetRowSkeleton into a new file. It does not use styled components anymore, so no need to have it here

Please also improve (props: { animate?: boolean }) so that it is unified with other components

tomasklim · 2024-10-04T10:17:57Z

packages/components/src/components/Table/Table.tsx

 };

-export const Table = ({ children, margin, colWidths }: TableProps) => {
+export const Table = ({ children, margin, colWidths, highlightRowOnHover = false }: TableProps) => {


Please add highlightRowOnHover to storybook

tomasklim

You have type check error

btw you can unify usage of isAnimating, animate, isAnimated,.. to something like shouldAnimate

enjojoy · 2024-10-04T13:28:49Z

You have type check error

btw you can unify usage of isAnimating, animate, isAnimated,.. to something like shouldAnimate

shouldAnimate is a hookFunction so I can't use that again, but I agree that it's odd with these similar namings, not sure how to tackle it here though

enjojoy requested a review from tomasklim September 30, 2024 12:06

enjojoy force-pushed the feat/use-table-at-dashboard branch 2 times, most recently from 11ee733 to 4230c28 Compare September 30, 2024 16:04

enjojoy self-assigned this Sep 30, 2024

enjojoy force-pushed the feat/use-table-at-dashboard branch 5 times, most recently from 910cf23 to 1c9500a Compare September 30, 2024 16:52

enjojoy marked this pull request as ready for review September 30, 2024 16:52

enjojoy requested review from jvaclavik and adamhavel as code owners September 30, 2024 16:52

adamhavel reviewed Sep 30, 2024

View reviewed changes

packages/suite/src/views/dashboard/AssetsView/AssetTable/AssetTableHeader.tsx Outdated Show resolved Hide resolved

packages/suite/src/views/dashboard/AssetsView/AssetTable/AssetTableHeader.tsx Outdated Show resolved Hide resolved

tomasklim requested changes Sep 30, 2024

View reviewed changes

enjojoy force-pushed the feat/use-table-at-dashboard branch 2 times, most recently from 90c8ae5 to dc9e6cf Compare September 30, 2024 21:49

enjojoy force-pushed the feat/use-table-at-dashboard branch from dc9e6cf to a300c52 Compare October 1, 2024 12:41

enjojoy changed the title ~~feat(suite): change assets grid to table~~ feat(suite): change assets table Oct 1, 2024

enjojoy force-pushed the feat/use-table-at-dashboard branch from a300c52 to 87903a8 Compare October 1, 2024 14:26

enjojoy force-pushed the feat/use-table-at-dashboard branch from 87903a8 to 5e55b21 Compare October 1, 2024 15:46

adamhavel reviewed Oct 1, 2024

View reviewed changes

enjojoy force-pushed the feat/use-table-at-dashboard branch 4 times, most recently from 7058104 to 040476c Compare October 2, 2024 10:52

tomasklim requested changes Oct 2, 2024

View reviewed changes

enjojoy force-pushed the feat/use-table-at-dashboard branch 5 times, most recently from f94d796 to de8c1f5 Compare October 3, 2024 12:57

enjojoy force-pushed the feat/use-table-at-dashboard branch from de8c1f5 to d24bb9a Compare October 3, 2024 13:10

enjojoy force-pushed the feat/use-table-at-dashboard branch from d24bb9a to 579a111 Compare October 3, 2024 13:40

enjojoy requested a review from tomasklim October 3, 2024 13:46

adamhavel reviewed Oct 3, 2024

View reviewed changes

enjojoy force-pushed the feat/use-table-at-dashboard branch from 579a111 to db0f5d6 Compare October 3, 2024 15:19

tomasklim requested changes Oct 4, 2024

View reviewed changes

enjojoy force-pushed the feat/use-table-at-dashboard branch 3 times, most recently from d882fe9 to 850042f Compare October 4, 2024 12:44

tomasklim approved these changes Oct 4, 2024

View reviewed changes

enjojoy force-pushed the feat/use-table-at-dashboard branch from 850042f to ee3fc07 Compare October 4, 2024 12:56

enjojoy force-pushed the feat/use-table-at-dashboard branch from ee3fc07 to c31ee1a Compare October 4, 2024 13:37

enjojoy added 2 commits October 4, 2024 15:19

feat(suite): highlight row on hover

658782e

feat(suite): rewrite asset table with new componen

68bf7be

enjojoy force-pushed the feat/use-table-at-dashboard branch from c31ee1a to 68bf7be Compare October 4, 2024 14:19

enjojoy enabled auto-merge (rebase) October 4, 2024 14:20

enjojoy merged commit 21955fd into develop Oct 4, 2024
21 checks passed

enjojoy deleted the feat/use-table-at-dashboard branch October 4, 2024 14:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(suite): change assets table #14625

feat(suite): change assets table #14625

enjojoy commented Sep 30, 2024 •

edited

Loading

tomasklim left a comment

adamhavel commented Oct 1, 2024

tomasklim commented Oct 1, 2024

enjojoy commented Oct 1, 2024

adamhavel commented Oct 1, 2024

enjojoy commented Oct 1, 2024

adamhavel Oct 1, 2024

adamhavel Oct 1, 2024

adamhavel Oct 1, 2024

enjojoy Oct 1, 2024

tomasklim left a comment •

edited

Loading

enjojoy commented Oct 3, 2024

enjojoy commented Oct 3, 2024

enjojoy commented Oct 3, 2024

enjojoy commented Oct 3, 2024

enjojoy commented Oct 3, 2024

adamhavel Oct 3, 2024

tomasklim left a comment

tomasklim Oct 4, 2024

enjojoy Oct 4, 2024

tomasklim Oct 4, 2024

enjojoy Oct 4, 2024

tomasklim left a comment

enjojoy commented Oct 4, 2024

		@@ -0,0 +1,25 @@
		import React, { createContext, useContext } from 'react';

		@@ -221,28 +117,28 @@ export const AssetRowSkeleton = (props: { animate?: boolean }) => {
		const animate = props.animate ?? shouldAnimate;

feat(suite): change assets table #14625

feat(suite): change assets table #14625

Conversation

enjojoy commented Sep 30, 2024 • edited Loading

Description

Related Issue

Screenshots:

tomasklim left a comment

Choose a reason for hiding this comment

adamhavel commented Oct 1, 2024

tomasklim commented Oct 1, 2024

enjojoy commented Oct 1, 2024

adamhavel commented Oct 1, 2024

enjojoy commented Oct 1, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomasklim left a comment • edited Loading

Choose a reason for hiding this comment

enjojoy commented Oct 3, 2024

enjojoy commented Oct 3, 2024

enjojoy commented Oct 3, 2024

enjojoy commented Oct 3, 2024

enjojoy commented Oct 3, 2024

Choose a reason for hiding this comment

tomasklim left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomasklim left a comment

Choose a reason for hiding this comment

enjojoy commented Oct 4, 2024

enjojoy commented Sep 30, 2024 •

edited

Loading

tomasklim left a comment •

edited

Loading