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

Echarts implementation #536

Merged
merged 29 commits into from
Aug 18, 2023
Merged

Echarts implementation #536

merged 29 commits into from
Aug 18, 2023

Conversation

AmbossKeegan
Copy link
Collaborator

@AmbossKeegan AmbossKeegan commented Jun 6, 2023

  • Moved all BarCharts to echarts and removed old BarCharts
  • Moved all HorizontalBarCharts to echarts and removed old HorizontalBarCharts
  • Created the Sankey chart as a replacement for the chord graph
  • Removed Visx and the Chord graph
Screen Shot 2023-06-05 at 8 09 11 PM Screen Shot 2023-06-27 at 2 47 12 PM

@secondl1ght secondl1ght self-requested a review June 7, 2023 20:37
Copy link
Collaborator

@secondl1ght secondl1ght left a comment

Choose a reason for hiding this comment

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

Ok nice work, after my review I have a few suggestions/questions:

  • we should probably change the minimum Y Axis interval to 1 because you can't have a fraction of a payment or invoice
  • the time scale displayed on the X Axis is different from the original chart, not sure if this matters?
  • it looks to me like the Payments chart is not working (I made a payment but nothing was displayed when I toggled the chart however it's working on master)
  • there is an error in the console about server rendering (this is also on master but might be an opportunity to fix it now)
  • the tooltip styling is different from the original chart (the original is much more minimal which I like better but it is personal preference)
  • rather than trying to create a component for this Bar Chart we could just place the code inline in the desired file (like we talked about on the call today this approach is more flexible)
  • the legend and main titles don't match the dropdown values for example Amount/Tokens vs. Count/Amount
  • the previous chart did not have a Title or Legend, do we want to keep the new chart also minimal in appearance?

Please let me know if any of my comments are not clear, I can provide screenshots or more details if needed.

@secondl1ght secondl1ght self-requested a review June 12, 2023 19:52
Copy link
Collaborator

@secondl1ght secondl1ght left a comment

Choose a reason for hiding this comment

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

My comments are mostly nits at this point so I think it's ready for @apotdevin to take a final review now and see if he is happy with it.

  • do we need a y label if its the same as the title?
  • make title copy match dropdown copy for consistency
  • is there a date format standard on TH like there is for Amboss? (for example to use in the tooltip dates)
  • we could format the volume numbers in the tooltip to make them more readable by adding commas (we could port the function over from Amboss)
  • I like the colors/style of the original tooltip but again this is just a personal preference or nit

Copy link
Owner

@apotdevin apotdevin left a comment

Choose a reason for hiding this comment

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

  • Let's reduce the padding the chart has - seems to big right now
  • I agree with Matt that we probably don't need the y-axis if the title is the same
  • Agree that mumbers with commas are easier to read!

src/client/next.config.js Outdated Show resolved Hide resolved
src/client/src/components/generic/helpers.tsx Outdated Show resolved Hide resolved
@secondl1ght secondl1ght mentioned this pull request Jun 13, 2023
@secondl1ght secondl1ght self-requested a review July 3, 2023 19:46
Copy link
Collaborator

@secondl1ght secondl1ght left a comment

Choose a reason for hiding this comment

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

Nice, left some comments/suggestions inline!

@apotdevin
Copy link
Owner

  • Padding around this chart is weird, lets try to center all charts inside the card.
image
  • Same here. Can we center and add the x-axis. There also seems to be an extra empty category?
image
  • For the y-axis can we format numbers similar to amboss space? 1800000 -> 1.8m
image
  • This chart get's cut off
image

@apotdevin
Copy link
Owner

What is needed to finish this PR @AmbossKeegan ?

@AmbossKeegan
Copy link
Collaborator Author

What is needed to finish this PR @AmbossKeegan ?

Just pushed a commit with code that bounds the sankey data inside its chart. Also provided a mock-data function that can be used to generate an arbitrarily large sankey dataset for testing purposes.

As far as I am aware, this was the last outstanding piece of feedback that needed to be addressed for this PR.

@apotdevin apotdevin enabled auto-merge (squash) August 18, 2023 16:09
@apotdevin apotdevin merged commit 0f893bd into master Aug 18, 2023
1 check passed
@apotdevin apotdevin deleted the echarts-implementation branch August 18, 2023 16:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants