Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

feat: linked graph view with navigation #121

Merged
merged 7 commits into from
Apr 25, 2024
Merged

Conversation

gaspardthrl
Copy link
Collaborator

@gaspardthrl gaspardthrl commented Apr 25, 2024

What I did

I linked the graph view with the rest of the app.
I implemented some basic navigation and search functionalities.
I also extracted part of the code from ForceDirectedGraph.tsx and implemented it in another file to have better modularity over components.

How I did it

I added the component inside the ContactGraph.tsx file and added some arguments to ForceDirectedGraph to implement navigation logic.

How to verify it

  1. Run the application
  2. Go in the Explore section
  3. Go in the Graph View mode
  4. Switch to the plain view mode
  5. Switch again to the graph view mode, the layout should not have changed given that you did not modify the drag by dragging or scrolling.
  6. Try to interact with the graph
  7. Typing in the search bar numbers from 0 to 10 included should "highlight" corresponding nodes
  8. When clicking return on the keyboard, if at least one node is selected, you should be redirected to an external profile screen dummy screen
  9. When keyboard is up and you click outside of it, the keyboard should disappear
  10. When clicking on a node an animation should start and a modal should appear
  11. Clicking on the node's profile picture displayed on the modal should redirect to an external profile screen dummy screen

Demo video

Conversion.video.final.1.mp4

Pre-merge checklist

The changes I have introduced:

  • work correctly
  • do not break other functionalities
  • work correctly on Android
  • are fully tested

@gaspardthrl gaspardthrl added the enhancement New feature or request label Apr 25, 2024
@gaspardthrl gaspardthrl self-assigned this Apr 25, 2024
@gaspardthrl gaspardthrl marked this pull request as ready for review April 25, 2024 09:22
@GaelCondeLosada
Copy link
Collaborator

Don't forget to address sonar cloud issues.

@gaspardthrl
Copy link
Collaborator Author

Don't forget to address sonar cloud issues.

Yes, don't know why I didn't see those earlier...

Thanks!

@gaspardthrl gaspardthrl changed the title feat: Linked Graph View with navigation feat: linked graph view with navigation Apr 25, 2024
Copy link
Collaborator

@GaelCondeLosada GaelCondeLosada left a comment

Choose a reason for hiding this comment

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

Overall good work ! I requested a few changes in the code. I also have a few suggestions on the UI.
First I think it could be good to push the modal upwards so we can still use the navigation bar when having a modal on screen:

We'll need the usage of regex, but will probably implement it later:

The graph goes under the present UI. I think this was done on purpose but it personally makes me feel a bit weird (it's maybe just me). Here is the screen I got when opening graph view for the first time:

Aside from that everything is good and everything works on my android device (I validated the works on Android check box) !

@gaspardthrl
Copy link
Collaborator Author

Overall good work ! I requested a few changes in the code. I also have a few suggestions on the UI. First I think it could be good to push the modal upwards so we can still use the navigation bar when having a modal on screen:

We'll need the usage of regex, but will probably implement it later: The graph goes under the present UI. I think this was done on purpose but it personally makes me feel a bit weird (it's maybe just me). Here is the screen I got when opening graph view for the first time: Aside from that everything is good and everything works on my android device (I validated the works on Android check box) !

Thanks for these insights!

Regarding the first comment, I don't think it makes much sense, take the example of Netflix, when you click on a movie and access the "modal" with its informations you need to quit the "modal" to browse through the website / app again.

Regarding the second comment about regex, I agree and had a TODO mentioning "fuzzy matching / similarity-based techniques" but sonar-cloud considered the TODO to be an issue...

About the last comment, I also agree and modified the view to make it less "all over the place"

Copy link
Collaborator

@AlbertoCentonze AlbertoCentonze left a comment

Choose a reason for hiding this comment

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

Very nice PR.

As more research bars stack-up in the project we will probably need to add some debouncer to avoid having our phones lagging when tapping on the keyboard. Especially in your case this should probably be prioritary for the next sprint!

Copy link

@gaspardthrl gaspardthrl merged commit 90f8e95 into main Apr 25, 2024
3 checks passed
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
enhancement New feature or request
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants