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

Bugfix: hide artifacts when setting changes #456

Merged
merged 8 commits into from
Mar 14, 2024

Conversation

brandonreid
Copy link
Contributor

No description provided.

@brandonreid brandonreid requested a review from a team as a code owner March 13, 2024 19:09
Copy link

netlify bot commented Mar 13, 2024

Deploy Preview for prefect-graphs ready!

Name Link
🔨 Latest commit c1ff30e
🔍 Latest deploy log https://app.netlify.com/sites/prefect-graphs/deploys/65f314b783917c0008bc7e76
😎 Deploy Preview https://deploy-preview-456--prefect-graphs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@pleek91
Copy link
Collaborator

pleek91 commented Mar 13, 2024

@brandonreid For things like edges we actually remove them rather than just hiding them. Is there a specific reason you're hiding them for this?

@brandonreid
Copy link
Contributor Author

@pleek91 Updated to remove artifacts and events when either are hidden.

Originally, given that these are rendered in multiple places (root run, on subflows, on nodes) it felt like less places to maintain hiding things to use visibility on the element's factory rather than each parent checking and removing. After trying it though, it actually cleans up some things for the parents and seems better, nice to not have hidden things on the stage as well.

artifactsContainer = new Container()
}

container.addChild(artifactsContainer)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did double check this. Adding something to a container that has already been added won't add a duplicate instance or anything. Pixi will shift it to the bottom of the children list, nbd though as we're generally assigning z-indexes for these anyway. Looked around to make sure toggling doesn't shift anything behind other things unintentionally and all looks good.

Copy link
Collaborator

@pleek91 pleek91 left a comment

Choose a reason for hiding this comment

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

LGTM!

@@ -107,7 +107,7 @@ export async function flowRunArtifactFactory<T extends ArtifactFactoryOptions>(o
const centeredX = x - (element.width - selectedOffset) / 2
const y = application.screen.height
- (element.height - selectedOffset)
- (flowHasEvents ? eventTargetSize : flowStateSelectedBarHeight)
- (flowHasEvents && !settings.disableEvents ? eventTargetSize : flowStateSelectedBarHeight)
Copy link
Collaborator

Choose a reason for hiding this comment

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

super nit: I'd try and avoid turnaries inside of logical operations for readability purposes. Creating a cost for eventsOffset or something would make this much easier to grok.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I appreciate all the nits dood. Updated. 🙏

@brandonreid brandonreid merged commit ac5b567 into main Mar 14, 2024
3 of 4 checks passed
@brandonreid brandonreid deleted the fix/artifacts-visibility-toggle branch March 14, 2024 15:16
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.

2 participants