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

Feature/bubbles revised 2 #283

Merged
merged 24 commits into from
Apr 25, 2024
Merged

Feature/bubbles revised 2 #283

merged 24 commits into from
Apr 25, 2024

Conversation

mikekucera
Copy link
Contributor

@mikekucera mikekucera commented Apr 10, 2024

General information

Associated issues: #264
Associated PR (this PR extends the previous PR): #274

Checklist

Author:

  • One or more reviewers have been assigned.
  • Automated tests have been included in this pull request, if possible, for the new feature(s) or bug fix.
  • The associated GitHub issues are included (above).
  • Notes have been included (below).

Reviewers:

  • All automated checks are passing (green check next to latest commit).
  • At least one reviewer has signed off on the pull request. Reviewers have two business days to review the pull request, after which the author may merge in the pull request unilaterally.

Notes

This PR branches off from Max's PR here: #274
This is a separate PR because it contains many changes that need to be discussed.

This PR does the following...

  • Reverts some of the style changes from PR #274

    • The issues with label casing have been addressed by using the 'description' field, so the uppercasing is no longer needed.
    • Moved labels back to the center of the nodes. There was too much overlapping of labels with other nodes when they were moved to the top of the node.
  • Reverts the behaviour where clicking away from a cluster collapses it.

    • We discussed this on a status call. There are a few issues with this behaviour
      • If the user wants to expand multiple clusters for exporting network images they won't be able to do that.
      • Clicking on the background is overloaded, it deselects nodes and collapses clusters, this makes it very hard to deselect the nodes in the cluster because you end up collapsing the cluster instead.
  • New behaviour:

    • There is now a prominent Expand/Collapse button at the bottom of each cluster. Currently it shows up when hovering over the compound node (not the bubble).
    • The button is below all the nodes in the cluster, so it no longer overlaps the nodes in the cluster. It also has different colors. This makes it much more obvious.
    • The button actually says "Expand" or "Collapse", hopefully making its use more clear.
    • The only way to expand/collapse is to click the button. Clicking the compound node is only used to drag or select. Clicking the bubble does nothing. The intention is that this will be more clear since Expand/Collapse is now a very explicit gesture that requires clicking a button. None of the network gestures are overloaded.
  • Issues:

    • Placing the button at the bottom-center of the clusters requires a change to cytoscape-layers extension. This PR currently pulls a modified version of cytoscape-layers from my personal NPM account. If we like these buttons we'll have to contribute the change back to cytoscape-layers.
    • The buttons look kind of basic, we can discuss how they should look.
    • Hovering doesn't work on mobile.
      • We can discuss strategies for mobile. For example the cluster closest to the center could have the button appear automatically.
Screenshot 2024-04-10 at 2 01 19 PM Screenshot 2024-04-10 at 2 01 33 PM

maxkfranz and others added 17 commits March 11, 2024 14:08
- #264
- When you click a bubble, it should show you the genes in the bubble. Basically, select the children.
- Drag anywhere on the cluster to move it.
- All uppercase labels instead of lowercase for now. It's better than lowercase, and getting everything nicely in title case requires cleaning the database.
- Bubbles shouldn't stay expanded. They should auto-close when you click away.
- When you expand, if it expands and overflows the screen it should it to screen.
…en if the whitespace is in the compound hit area rectangle #264
Bypass the bubblesets implementation and just copy the SVG and transform it manually during the drag to keep things fast.

Revised bubble/cluster behaviour #274
- If you click a bubble where it overlaps with an edge, the bubble will get the event.
- Compound labels are on top of edges and easier to read.
@mikekucera
Copy link
Contributor Author

It looks like the dev server doesn't like my custom build of cytoscape-layers. I have no idea how to fix the issue. To test this branch please check it out locally.

Copy link
Member

@chrtannus chrtannus left a comment

Choose a reason for hiding this comment

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

Since I don't have the modified version of cytoscape-layers, the cluster button appears at the canvas' top-left corner, too far from the cluster, so I cannot click and test it.
Other than that, I like the idea, though we might have to tweak the button style a bit more, as Mike mentioned.

@mikekucera
Copy link
Contributor Author

did you try running 'npm install' ?

@chrtannus
Copy link
Member

Yes, but "npm i" did not fix it.

@mikekucera
Copy link
Contributor Author

The issue is that in this branch EM-web and the bubble-clusters extension both depend on different versions of cytoscape-layers. I have a fairly recent version of node on my computer that seems to support this. I bet you and the dev server have older versions of node.

@mikekucera
Copy link
Contributor Author

  • Moved the button back to the center of the cluster.
    • This mitigates button placement issues with irregularly shaped clusters.
    • Removes need for custom patched cytoscape-layers extension.
  • Button now automatically hides itself after a 3 second timeout.
    • Mitigates issue where button overlaps nodes under it.
Screenshot 2024-04-11 at 12 48 54 PM Screenshot 2024-04-11 at 12 49 08 PM

@mikekucera
Copy link
Contributor Author

I changed the behaviour a bit to support touch devices better.

  • Hovering the mouse cursor over a cluster causes the button to show. It auto-hides after 3 seconds. To show the button again you have to move the cursor away from the cluster then back over it again. That way the button doesn't interfere too much with the nodes under it.
  • Panning the network causes the cluster in the center to show its button. This is so that the button will show up on touch devices where mouse hover isn't available.
  • Its probably not reliable to test for mobile to change the behaviour. The user could be on a windows laptop with a touch screen. That's why mouse cursor hover and panning are both enabled at the same time.

With mouse...

Screen.Recording.2024-04-18.at.10.39.49.AM.mov

With touch...

Screen.Recording.2024-04-18.at.10.42.32.AM.mov

@mikekucera mikekucera merged commit 82deb72 into main Apr 25, 2024
1 check passed
@chrtannus chrtannus deleted the feature/bubbles-revised-2 branch June 5, 2024 15:08
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