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

[Batch Viewer] Fix multiple AMM's interactions not visible #78

Merged
merged 1 commit into from
May 4, 2022

Conversation

ramirotw
Copy link
Contributor

@ramirotw ramirotw commented May 2, 2022

Summary

Closes #72

image

To Test

Open the Batch Viewer in https://pr78--gpui.review.gnosisdev.com/tx/0x8bb4f9644bdf19684d66c55133ded17c198e36069a502f903311eb07ed978145 and there should be multiple interactions in the Balancer's Vault

@coveralls
Copy link

coveralls commented May 2, 2022

Pull Request Test Coverage Report for Build 2257766771

Warning: This coverage report may be inaccurate.

We've detected an issue with your CI configuration that might affect the accuracy of this pull request's coverage report.
To ensure accuracy in future PRs, please see these guidelines.
A quick fix for this PR: rebase it; your next report should be accurate.

  • 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.267%

Totals Coverage Status
Change from base Build 2247719314: 0.0%
Covered Lines: 2170
Relevant Lines: 4173

💛 - Coveralls

@github-actions
Copy link

github-actions bot commented May 2, 2022

@ramirotw ramirotw changed the title [Batch Viewer] Fix multiple AMM interaction not visible [Batch Viewer] Fix multiple AMM's interactions not visible May 2, 2022
@@ -25,7 +25,7 @@ export function STYLESHEET(theme: DefaultTheme): Stylesheet[] {
width: 2,
'target-arrow-shape': 'triangle',
'target-arrow-color': theme.grey,
'curve-style': 'unbundled-bezier',
'curve-style': 'bezier',
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure about this change:
image
Before (with: 'curve-style': 'unbundled-bezier' )
image

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 it looks worse but without it you keep having the same reported issue. Could you see if there's a way to make them look similar? Maybe we could suggest merging the 2 arrows into 1 as we discussed.

@elena-zh
Copy link

elena-zh commented May 3, 2022

Hey @ramirotw , great changes!
But I'd agree with @alongoni that overlapping elements do not look good. Moreover, they are not adjustable when zoom in/out the diagram.
Mobile view:
image

Also, I have noticed that ETH is displayed as WETH on the diagram:
eth

@ramirotw
Copy link
Contributor Author

ramirotw commented May 3, 2022

But I'd agree with @alongoni that overlapping elements do not look good. Moreover, they are not adjustable when zoom in/out the diagram.
Mobile view:
image

Yes, we still need to improve the layout, even more on mobile. I have some ideas to reduce the labels and arrows count and combine them into 1 bidirectional arrow having a sellToken<>buyToken label. For batches like the above that will improve things, but we still need research on better ways to organize the nodes, we have a ticket for that and will probably be for the next sprint.

Also, I have noticed that ETH is displayed as WETH on the diagram:
eth

It's indeed WETH:

{
  "owner": "0x8668eb7a9cde59618f4c187415641063c02f41c2",
  "sellToken": "0xc02aaa39b223fe8d0a0e5c4f27ead9083c756cc2",
  "buyToken": "0xa0b86991c6218b36c1d19d4a2e9eb0ce3606eb48",
  "sellAmount": "3481348765869705828",
  "buyAmount": "12125367336",
  "feeAmount": "12181348040899292",
  "orderUid": "0x686dd49dcd2d6b4b3ad49e18a3c8c1c130d3f6a1d66103936c20591ef98a831d8668eb7a9cde59618f4c187415641063c02f41c2624ae6eb"
}

@ramirotw
Copy link
Contributor Author

ramirotw commented May 4, 2022

@elena-zh are you ok to merge this and work on improving the general layout and overlapping in the new sprint?

@elena-zh
Copy link

elena-zh commented May 4, 2022

@ramirotw , I'm not sure about it.
Actually, if me merge this, it will go to the release. And we will receive user's complaints on it, I guess.
But let ask @alfetopito opinion about this.

@alfetopito
Copy link
Collaborator

I think it's worth to move on as is since it fixes one bug, albeit making the visualization a bit harder in some cases.

@ramirotw ramirotw merged commit d96287f into develop May 4, 2022
@ramirotw ramirotw deleted the ramirotw/issue-72-batch-viewer-nodes branch May 4, 2022 14:14
@henrypalacios henrypalacios mentioned this pull request May 4, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Batch Viewer] AMMs nodes are displayed only once when involved in multiple interactions
5 participants