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

Toogle labels option #814

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open

Conversation

Palt
Copy link
Contributor

@Palt Palt commented Aug 27, 2024

Description

Added toggles to the SettingsMenu with the options to toggle the labels of Nodes and Edges.

Motivation and Context

This PR addresses: #431

These changes will allow a user to toggle the labels and make it easier to hide potential sensitive data before taking a screenshot.

How Has This Been Tested?

Locally tested manually and using existing tests.

Screenshots (optional):

bloodhound_lable_toggle_menu

Types of changes

  • New feature (non-breaking change which adds functionality)

Checklist:

@urangel urangel added enhancement New feature or request user interface A pull request containing changes affecting the UI code. labels Aug 27, 2024
@irshadaj irshadaj marked this pull request as draft August 28, 2024 17:41
@StephenHinck
Copy link
Collaborator

Good morning, @Palt, and thank you for another contribution! I have some functional feedback on your PR before I ask our Engineering team to review the code.

  • The "Explore" portion of the product is one subset of BloodHound's functionality. As such, changes that impact only that portion of the application must live within the page and not at a global level. Our UX designer has provided the following mockup for the expected behavior of this function within the product within the graph context specifically (specifically the menu in the bottom-left corner):
    image
  • The graph elements should maintain a static position regardless of whether labels are displayed or hidden; the view should not reset when toggling the label elements on and off.
  • We would like to include options to show or hide all labels quickly.

I appreciate the effort you've put into this, and I recognize this is a decent bit of rework on the changes you have already made. We do have some upcoming work planned for our team, where they can help take this across the line if you'd rather not rework!

On a related note, I would love to send you a swag package to show our gratitude for your contributions to the product. If you can send me an email at shinck (AT) specterops (DOT) io with your shipping address, I'll get that on its way!

@Palt
Copy link
Contributor Author

Palt commented Aug 30, 2024

Hi @StephenHinck, and thank you for great feedback!

I’m happy to be able to contribute when I can as this is an awesome tool that I use quite a lot.

Reading through what you’ve written it is as you say, this is not the way to implement it. I’ll blame it on my lack of understanding of the project and will continue digging a bit deeper into it to get a better grasp on it.

I’ll see what I can do to see if I can implement the needed changes to get it to a state where you can agree with it.

Regarding Swag: Thank you! I’ve sent you an email.

@Palt Palt marked this pull request as ready for review August 30, 2024 16:16
@Palt
Copy link
Contributor Author

Palt commented Aug 30, 2024

I've re-done the PR and would say that it now aligns with what you described @StephenHinck. It is now a smaller change and far less complex from previous solution.

Hope it is what you are looking for.

bloodhound_lable_toggle_menu

@irshadaj irshadaj added the external This pull request is from an external contributor label Sep 11, 2024
@elikmiller
Copy link
Collaborator

@StephenHinck are the updates from @Palt what you are looking for? If so we can start getting this code reviewed.

@StephenHinck
Copy link
Collaborator

@StephenHinck are the updates from @Palt what you are looking for? If so we can start getting this code reviewed.

From a functional perspective, this matches our AC. The ticket is up at BED-4613 internally!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request external This pull request is from an external contributor user interface A pull request containing changes affecting the UI code.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants