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

feat: add single dependency graph view for a specific resource relations #1161

Open
wants to merge 15 commits into
base: develop
Choose a base branch
from

Conversation

shavidze
Copy link
Contributor

@shavidze shavidze commented Nov 3, 2023

Solution

Add a single dependency graph view for a specific resource relations in the inventory tabs

Checklist

  • Code follows the contributing guidelines
  • Changes have been thoroughly tested
  • Documentation has been updated, if necessary
  • Any dependencies have been added to the project, if necessary

Reviewers

@Traxmaxx

@Azanul
Copy link
Collaborator

Azanul commented Nov 3, 2023

@shavidze Could you resolve the conflicts before we get to the review

@shavidze
Copy link
Contributor Author

shavidze commented Nov 7, 2023

don't merge before this

@Azanul Azanul self-requested a review November 7, 2023 13:28
Copy link
Collaborator

@Azanul Azanul left a comment

Choose a reason for hiding this comment

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

  • The "open in explorer" link doesn't seem like a link, it acts as text on hover.
  • IDK if this is intended behaviour but on opening in explorer the filter option is not available.
  • On opening in explorer, only the selected resource and it's relations are on the graph. IMO it should have zoomed in on the resource instead of filtering everything else out. But the current one is also good.

@shavidze
Copy link
Contributor Author

  • The "open in explorer" link doesn't seem like a link, it acts as text on hover.
  • IDK if this is intended behaviour but on opening in explorer the filter option is not available.
  • On opening in explorer, only the selected resource and it's relations are on the graph. IMO it should have zoomed in on the resource instead of filtering everything else out. But the current one is also good.

you can see our conversation about this feature here

@Azanul
Copy link
Collaborator

Azanul commented Nov 16, 2023

  • The "open in explorer" link doesn't seem like a link, it acts as text on hover.
  • IDK if this is intended behaviour but on opening in explorer the filter option is not available.
  • On opening in explorer, only the selected resource and it's relations are on the graph. IMO it should have zoomed in on the resource instead of filtering everything else out. But the current one is also good.

you can see our conversation about this feature here

It seems like point 1 and 3 still stand.
Regarding 2nd point, I think @AllieMendes meant that there shouldn't be any applied filter but the option should still be available.

Explorer - Komiser

@shavidze
Copy link
Contributor Author

@Azanul @AllieMendes
What is the purpose of having a filter there?
We still don't have and can't have a filter that would filter by resource id.
If we had that, then the user would be able to display all the existing graphs on the dashboard but now I don't understand why we need these filters in this case.

@Azanul
Copy link
Collaborator

Azanul commented Nov 16, 2023

@AllieMendes correct me if I'm wrong.
On opening explorer page from resource, the usual explorer will open up with no filters and all other resources of the given provider. The only thing different would be that the selected resource will be centered and zoomed on.

@shavidze
Copy link
Contributor Author

@AllieMendes correct me if I'm wrong. On opening explorer page from resource, the usual explorer will open up with no filters and all other resources of the given provider. The only thing different would be that the selected resource will be centered and zoomed on.

hmm, Yes you're right, I'm wrong maybe I read the previous Allie's comment, I will update it today

@AvineshTripathi
Copy link
Collaborator

Hey @Azanul can we move forward for this PR?

@shavidze
Copy link
Contributor Author

Hey @Azanul can we move forward for this PR?

Hi, sorry I just left to zoom the actual relations on explore page, otherwise the main functionality is done.
Sorry again, I left my prev job and had a several interviews last days, tomorrow I believe I will have a time to update it.

@Traxmaxx
Copy link
Contributor

@shavidze no problem. Take all the time you need. Do we know that is still left to be done in this PR?
I hope your interviews are going well! 🙏

@shavidze
Copy link
Contributor Author

@shavidze no problem. Take all the time you need. Do we know that is still left to be done in this PR? I hope your interviews are going well! 🙏

Thanks for your understanding, and sorry again.
I updated my PL and merged conflicts, one thing left, when we open the explorer page I'm trying to save the resource-id in the browser's local storage, maybe it would be better to handle it with some state but I think local storage would be okay as well, since if we try to pass it in the URL query it messes some things up.
I was trying to get the node with this ID but it seems it does not work correctly and I don't have enough time to read Cytoscape docs, if someone will help me I would be honored.

@Azanul
Copy link
Collaborator

Azanul commented Dec 4, 2023

@shavidze no problem. Take all the time you need. Do we know that is still left to be done in this PR? I hope your interviews are going well! 🙏

Thanks for your understanding, and sorry again. I updated my PL and merged conflicts, one thing left, when we open the explorer page I'm trying to save the resource-id in the browser's local storage, maybe it would be better to handle it with some state but I think local storage would be okay as well, since if we try to pass it in the URL query it messes some things up. I was trying to get the node with this ID but it seems it does not work correctly and I don't have enough time to read Cytoscape docs, if someone will help me I would be honored.

@Traxmaxx if you got time

@jakepage91
Copy link
Contributor

Did you have time to take a look @Traxmaxx ?

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.

None yet

5 participants