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

Adding force-simulation layout algorithm to draw nodes #119

Merged
merged 9 commits into from
Jun 22, 2022

Conversation

henrypalacios
Copy link
Contributor

@henrypalacios henrypalacios commented Jun 10, 2022

Summary

Closes #114

Adding layout graph selector
Selection_582

To Test

Open any of the following transactions:

  1. 0xaa5d...baf0
  2. 0x0d6f...ca38
  3. 0xd7e3...5894
  4. 0xc247...ead4
  5. 0xf2a0...d522

@coveralls
Copy link

coveralls commented Jun 10, 2022

Pull Request Test Coverage Report for Build 2513019511

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 43.872%

Totals Coverage Status
Change from base Build 2465584360: 0.0%
Covered Lines: 2162
Relevant Lines: 4177

💛 - Coveralls

@github-actions
Copy link

@henrypalacios henrypalacios changed the title Adding 4 layout drawing algorithms Adding force-simulation layout algorithm to draw nodes Jun 15, 2022
@henrypalacios henrypalacios marked this pull request as ready for review June 15, 2022 13:04
@henrypalacios henrypalacios assigned alongoni and unassigned alongoni Jun 16, 2022
@henrypalacios henrypalacios requested a review from alongoni June 16, 2022 01:35
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 , great PR to play with!

Some issues and doubts from my side:

  1. Can we write layout names using upper and lower-case letters? I mean, as a stupid end-user, it was not clear to me what this or that type of diagram means. And when I googled, I found that fcose is actually fCoSE (easies to google it when know a certain name). And klay is KLayout..

  2. fCoSE layout always changes its position when press on 'reset' button. Maybe instead/or besides of the 'Reset button we should show 'Play' button if it works this way. WDYT?

  3. KLayout looks too weird with a lot of overlapped elements, especially for graphs with few nodes
    image
    image
    Maybe we should hide it for such graphs?

  4. In this transaction with klay layout 2 arrows are always blinking when press on the Reset button

  5. I think it would be nice to 'save' layout selection when switch between Graph/Orders tabs

Thanks

@ramirotw ramirotw requested a review from alfetopito June 21, 2022 11:50
@alfetopito
Copy link
Collaborator

First of all, very nice. It's very useful for some more unusual batches like the ones you shared.

Now, regarding Elena's points:

2. I'm not found of play as it indicates to me that it should play something, like a video. I would prefer re-arrange instead

3. Agree, looks weird, but I don't think we have enough data, and might not be worth the effort to try and identify when it should be hidden. I'd leave it up to the user to pick a different layout if they chose to

5 Out of the scope of this task, and might not make sense, as every layout might fit better a different type of settlement. I think the grid is the best use case for the majority of cases and as it's already the default.

And some points from me:

L1: Not very found of the animation on load. Can it be disabled?
L2: The table seems to 'jump' when switching between graph and list. That's not in the video, but the jump is because the graph causes the browser to have a scroll bar, which is removed when I go to the list.
Even on one batch where it has a scroll bar there's some jumping

Screen.Recording.2022-06-21.at.18.01.10.mov

L3: I understand the layouts match the algorithms, but does it make sense from a user point of view? I don't have a good suggestion here, just find it not relevant and would prefer a name more meaningful, like grid.

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.

❤️

@elena-zh
Copy link

Hey @henrypalacios , great changes!
I have found some low-prio UI issues, but reported them as separate tasks in order to fix it later:

Thanks

@elena-zh elena-zh self-requested a review June 22, 2022 11:15
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.

StandingOvationGIF

@henrypalacios
Copy link
Contributor Author

Thanks for the feedback team,

I have found some low-prio UI issues, but reported them as separate tasks in order to fix it later:

  1. issues with Reset button will b solved by disabling it for a default BV view [Batch Viewer] Disable the reset button #101 (comment)
  2. BV diagram blinks when loaded #134
  3. I think, it would be nice to place a layout button to another place in a mobile view [BV] Change a layout dropdown position in a mobile view #135 . But is the current position looks good to all the rest team members, this issue might be closed.

@elena-zh 👇

  1. I tried to fix the Klayout blink, but definitely the best way to fix it is [Batch Viewer] Disable the reset button #101.

  2. I was going to comment on the BV diagram blinks when loaded #134. Because I still do not understand why it does not take the layout in the first render, (maybe I'll go to the author of the library).

  3. It seems to me well, we will discuss it in [BV] Change a layout dropdown position in a mobile view #135.


L2: The table seems to 'jump' when switching between graph and list. That's not in the video, but the jump is because the graph causes the browser to have a scroll bar, which is removed when I go to the list.
Even on one batch where it has a scroll bar there's some jumping

There is still pending the jump that @alfetopito found, with a big tx like 0xf2a00ef766bc5caee96cbee8e1842e8ed29ca5059b1a5daa71258d6232d1d522, I'll see it with @alongoni in another PR.


  1. I think it would be nice to 'save' layout selection when switch between Graph/Orders tabs

☝️ regarding this last point, I think it would be great to use a URLQueryParam like (&layout=grid) that would also allow to share the tx with a specific layout (it would be a task outside this PR).

@henrypalacios henrypalacios merged commit 6c22611 into develop Jun 22, 2022
@henrypalacios henrypalacios deleted the 114-bv-layout-selector branch June 22, 2022 14:36
@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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use an algorithm to draw a graph (force-directed)
6 participants