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

Vertical blocks #205

Merged
merged 17 commits into from
Apr 7, 2023
Merged

Vertical blocks #205

merged 17 commits into from
Apr 7, 2023

Conversation

lubej
Copy link
Collaborator

@lubej lubej commented Mar 14, 2023 •
edited
Loading

Solution

Implement vertical view on blocks and transactions page

Links

https://www.loom.com/share/b816268e56ba4851b90509e4415d4696

@cloudflare-workers-and-pages
Copy link

cloudflare-workers-and-pages bot commented Mar 14, 2023 •
edited
Loading

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: 1fb6b34
Status:⚡️  Build in progress...

View logs

@lubej lubej force-pushed the ml/mobile-vertical-blocks branch 6 times, most recently from 43b8b06 to fd01eb7 Compare March 20, 2023 07:39
@lubej lubej force-pushed the ml/mobile-vertical-blocks branch 2 times, most recently from 3b27cda to a37a166 Compare March 21, 2023 16:50
@lubej lubej marked this pull request as ready for review March 22, 2023 08:34
@lubej lubej force-pushed the ml/mobile-vertical-blocks branch from df53498 to c967245 Compare March 23, 2023 07:31
lukaw3d
lukaw3d requested changes Mar 25, 2023
View reviewed changes
src/app/components/AppendMobileSearch/index.tsx
<Box>{children}</Box>

{action}
Copy link
Member

Choose a reason for hiding this comment

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

This is an odd place to put it. Mobile search floats above anyway right, so can't the button simply be in the footer, next to Typography?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Didn't found any smart solution to place the button in the center.

src/app/components/LoadMoreButton/index.tsx Show resolved Hide resolved
src/app/components/PageLayout/index.tsx Show resolved Hide resolved
src/app/components/SubPageCard/index.tsx Show resolved Hide resolved
src/app/components/Table/index.tsx Outdated Show resolved Hide resolved
src/app/components/TableViewSpeedDial/index.tsx Outdated Show resolved Hide resolved
src/app/pages/BlocksPage/index.tsx Outdated Show resolved Hide resolved
src/app/pages/BlocksPage/index.tsx Outdated
Comment on lines 104 to 110
{tableView === TableView.Vertical && (
<BlockDetails>
Copy link
Member

Choose a reason for hiding this comment

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

It bothers me that these vertical cards aren't just a different layout of a table. They are a complete second implementation of Blocks list and of Transactions list. It doubles complexity of every view that supports it.

Some limitations if it was implemented as a Table feature instead (converting columns into rows):

  • Cards would have to show the same information as the table shows. No new information like transaction's Gas limit that is not a column
  • And show it in the same order as columns are
  • Pagination
    • It would still have to leak implementation details into parent queries (limit, offset)
    • Show More could not be part of the footer

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Right, I agree it could have been done more general. But Didn't wan't to complicate the Table component. I suggest we keep it as is, and refactor, if we add the 3th view.

src/app/pages/TransactionsPage/index.tsx Outdated Show resolved Hide resolved
src/app/pages/BlocksPage/index.tsx Show resolved Hide resolved
@lubej lubej force-pushed the ml/mobile-vertical-blocks branch from 90d47ec to 9c17401 Compare March 29, 2023 13:35
@lubej lubej requested a review from lukaw3d March 29, 2023 13:37
@lubej lubej force-pushed the ml/mobile-vertical-blocks branch 2 times, most recently from 3b4337b to c438c76 Compare April 4, 2023 07:42
@lubej lubej force-pushed the ml/mobile-vertical-blocks branch from c438c76 to e4479ca Compare April 4, 2023 13:02
lubej
lubej commented Apr 4, 2023
View reviewed changes
src/app/pages/BlocksPage/index.tsx Show resolved Hide resolved
src/app/pages/BlocksPage/index.tsx Outdated Show resolved Hide resolved
@lubej lubej requested a review from lukaw3d April 4, 2023 13:45
@lubej lubej force-pushed the ml/mobile-vertical-blocks branch from 2115987 to 5b2e794 Compare April 6, 2023 10:29
@lubej lubej force-pushed the ml/mobile-vertical-blocks branch from 4e0fae3 to 92fd028 Compare April 6, 2023 12:19
@lubej lubej requested a review from lukaw3d April 6, 2023 12:30
lukaw3d
lukaw3d approved these changes Apr 6, 2023
View reviewed changes
src/app/pages/BlocksPage/index.tsx Outdated Show resolved Hide resolved
@lubej lubej force-pushed the ml/mobile-vertical-blocks branch from 8ae2aba to 1fb6b34 Compare April 7, 2023 07:08
@lubej lubej merged commit 4f09e54 into master Apr 7, 2023
@lubej lubej deleted the ml/mobile-vertical-blocks branch April 7, 2023 07:09
@lukaw3d lukaw3d mentioned this pull request Apr 13, 2023
Open
@buberdds buberdds mentioned this pull request Jun 13, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lukaw3d lukaw3d lukaw3d approved these changes

@buberdds buberdds Awaiting requested review from buberdds buberdds is a code owner

@csillag csillag Awaiting requested review from csillag csillag is a code owner

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

Successfully merging this pull request may close these issues.
2 participants
@lubej @lukaw3d