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

[Home Page] Volume chart #62

Merged
merged 31 commits into from
Apr 28, 2022
Merged

[Home Page] Volume chart #62

merged 31 commits into from
Apr 28, 2022

Conversation

henrypalacios
Copy link
Contributor

@henrypalacios henrypalacios commented Apr 9, 2022

Summary

Closes #37

Proposal:

To Test

@github-actions
Copy link

github-actions bot commented Apr 9, 2022

@henrypalacios henrypalacios changed the title 37 volume chart Volume chart Apr 11, 2022
@ramirotw ramirotw changed the title Volume chart [Home Page] Volume chart Apr 14, 2022
@henrypalacios henrypalacios marked this pull request as ready for review April 27, 2022 01:53
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 Henry! Quite a lot of code.

It's fine by me, just asked @fairlighteth for a review too as he's better equipped with the CSS bits.

@alfetopito
Copy link
Collaborator

Just realized that on mobile it's not looking so good
Screen Shot 2022-04-27 at 11 56 23

@fairlighteth
Copy link
Contributor

Thank you for this PR. Here some comments:

  • Is it possible to show generated surplus over the last 24h or 7d?
  • Would rename Cow volume => CoW Protocol volume
  • Mobile needs to be made responsive

@elena-zh
Copy link

Hey @henrypalacios , great job!
Some issues from my side:

  1. Could we make somehow an amount at the top not being strike through with the line?
    not overlap

  2. While loading data on the card, could we use the whole card loading effect instead of only 1 line loaded?
    the whole loading
    like this
    image

  3. As @alfetopito and @fairlighteth have mentioned above, it would be nice to make the diagram auto-adjustable when changing a screen width. Also, card's content should be auto-adjustable as well: https://watch.screencastify.com/v/P3ANnXlEwQ8eVFxqzPYu

  4. In a real mobile devices horizontal scroll appears on the Home page
    image
    I believe, we can remove it

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.

Nice!!

Regarding the code review, I'm still trying to understand everything, will probably comment later.

Regarding the functional part there are two things I'd like to comment:

  1. Shouldn't the graph color be green here instead of red? From my understanding if the first value of the selected period is $264 and the last $10.267.230 the trend is positive not negative.

image

2. I believe we should limit how tall the graph can be so the labels don't collide with any line

image

@ramirotw
Copy link
Contributor

  1. Another thing to discuss better: now the Cow Volume label is showing the last data point value, but as we discussed, shouldn't this be the average of the selected period? @alfetopito I summon you

@henrypalacios
Copy link
Contributor Author

henrypalacios commented Apr 27, 2022

Thank you for you time @fairlighteth,
regarding your comments:

  • Is it possible to show generated surplus over the last 24h or 7d?

It is no possible yet because the data is not available in the graph.

  • Would rename Cow volume => CoW Protocol volume

Done!

  • Mobile needs to be made responsive

@alongoni will work on this!

@ramirotw ramirotw self-requested a review April 27, 2022 19:32
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.

🚢

@henrypalacios
Copy link
Contributor Author

@elena-zh could you please check again 🙏?

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.

Hey @henrypalacios , @alongoni , changes LGTM!
I have created 2 related low-priority issues:

Besides, I would update the cards each time when I change a network. However, it must be fixed in another PR.

Thanks

@henrypalacios henrypalacios added the Auto-merge PRs with this tag will be automatically merged when approved and CI succeeds label Apr 28, 2022
@henrypalacios henrypalacios merged commit 1b8ea01 into 35-epic-home-page Apr 28, 2022
@henrypalacios henrypalacios deleted the 37-volume-chart branch April 28, 2022 16:22
@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.

6 participants