Skip to content
This repository has been archived by the owner on Feb 2, 2024. It is now read-only.

Adding statistics SummaryCards component #50

Merged
merged 22 commits into from
Apr 18, 2022

Conversation

henrypalacios
Copy link
Contributor

@henrypalacios henrypalacios commented Apr 5, 2022

Summary

Closes #38

Adding statistics SummaryCards component in order to achieve 👇

UI_Explorer v1 - HOME(rev2)

UI_Explorer v1 - HOME(rev2) --_ MOBILE

To Test

Observations

  • These components are UI only, which will be available to connect to real data.
  • Originally reported in gnosis/gp-ui/#1069

@coveralls
Copy link

coveralls commented Apr 5, 2022

Pull Request Test Coverage Report for Build 2106063322

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 44.106%

Totals Coverage Status
Change from base Build 2086337745: 0.0%
Covered Lines: 2155
Relevant Lines: 4150

💛 - Coveralls

@github-actions
Copy link

github-actions bot commented Apr 5, 2022

@henrypalacios henrypalacios changed the base branch from develop to 35-epic-home-page April 7, 2022 14:00
@elena-zh
Copy link

elena-zh commented Apr 12, 2022

Hey @henrypalacios , great job!
However, some notes from my side:

  1. Some cards in the Storybook show huge text on them:
    image

  2. Cards displaying does not support responsive views (in Storybook)
    not adjusted

  3. Based on the mock-up, cards should be placed under the header, but they are displayed under the search field. Is this expected for now?
    image

  4. Cards might be better aligned in a tablet views:
    tablet view

  5. Besides, seems that cards take a lot of room on the page in a tablet view, so I personally would love to make them a bit more compact

  6. it would be great to make card's text font brighter when highlight it: currently, it is almost invisible
    brighter

  7. Again, in comparison with the mock-up, 2 small cards with a margin between them should correspond to the height of one bigger card. In our current implementation it does not work this way.
    image

Were these changes discussed somewhere/OR we have an updated mock-uo of the Home page about which accidentally I'm not aware of?

Thank you!

@henrypalacios
Copy link
Contributor Author

Thanks for the feedback @elena-zh!

I'll start by giving you the context of your last question, since we won't have the card with the surdplus yet and while the token table at the bottom of the mock up is being built. Agustin proposed a temporary layout while some of the first ones are executed.

Regarding your notes:

1 .Some cards in the Storybook show huge text on them:
2. Cards displaying does not support responsive views (in Storybook)
I will work on it.

  1. Based on the mock-up, cards should be placed under the header, but they are displayed under the search field. Is this expected for now?
    yes, but this would be provisional in the proposed layout.

4, 5, 6
@alongoni here maybe you can help me

  1. ... 2 small cards with a margin between them should correspond to the height of one
    I'll review it.

@elena-zh
Copy link

Hey @henrypalacios , storybook cards look great now!
Maybe a nitpick, but it would be great to enhance text displaying on the last card when a screen width in [320-349] px
image

Cards on Explorer started to look much-much better. Maybe we could increase the graph widget's height to correspond to the neighboring card's height? WDYT?
image

Again, when a screen width is less than 349 px, cards start to overlap each other
image

Thanks!

Copy link
Collaborator

@alfetopito alfetopito left a comment

Choose a reason for hiding this comment

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

Looking great, loved the shimmer effect

Have some small comments in the code and same comment as Elena in regards to the graph view
Screen Shot 2022-04-13 at 16 35 40
It's a bit shorter than the rest of the cards

@alongoni
Copy link
Contributor

hey @elena-zh I've fixed some issues:
Selection color: (It will affect all the app)
image
Card on storybook. 340px width resolution:
image

Copy link

@elena-zh elena-zh left a comment

Choose a reason for hiding this comment

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

@henrypalacios , @alongoni , LGTM now!

Copy link
Contributor

@ramirotw ramirotw left a comment

Choose a reason for hiding this comment

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

Really nice!

src/utils/mediaQueries.ts Show resolved Hide resolved
src/utils/mediaQueries.ts Outdated Show resolved Hide resolved
Copy link
Collaborator

@alfetopito alfetopito left a comment

Choose a reason for hiding this comment

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

Approved with minor comments

"batchInfo": { "lastBatchDate":1649881035, "batchId": "0x0003723b9eb1598e12d15c69206bf13c971be0310fa5744cf9601ed79f89e29c" },
"dailyTransactions": {"now": 612, "before": 912},
"totalTokens": 193,
"dailyFees": {"now": 55225.61205047748511254485049406426, "before": 40361.20651840192742698089787142249}
Copy link
Collaborator

Choose a reason for hiding this comment

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

One more thing about the shape of the data.

Usually it's not a good idea to work with floats in JS and also if working with big numbers JS number type won't be able to handle it.

Having said that, it might not be a problem for this use case as:

  1. We are simply displaying data without any further calculations, so loss of precision won't be really a problem
  2. I expect fees to not be large enough to go over JS number limit
  3. It mostly depends on how the graph response will look like, which might not be yet known

So, keep that in mind for the future, but does not need necessarily to change anything at this point

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I have it in mind! I would think they come as a string and may need to be formatted, but we can define it in #40 when integrating the SDK.

@henrypalacios henrypalacios added the Auto-merge PRs with this tag will be automatically merged when approved and CI succeeds label Apr 14, 2022
@alfetopito alfetopito merged commit 5347984 into 35-epic-home-page Apr 18, 2022
@alfetopito alfetopito deleted the 38-stats-card-ui branch April 18, 2022 11:35
@henrypalacios henrypalacios mentioned this pull request Jun 23, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
app:Explorer Explorer App Auto-merge PRs with this tag will be automatically merged when approved and CI succeeds
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[HomePage] - Stats cards UI
6 participants