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

Make command clicking on a node in asset lineage graph open it in a new page #21456

Conversation

salazarm
Copy link
Contributor

Summary & Motivation

Adding real links here to do this rather than using a JS click handler.

How I Tested These Changes

Screen.Recording.2024-04-26.at.1.57.05.PM.mov

Comment on lines 173 to 176
<AssetNode
definition={graphNode.definition}
selected={graphNode.id === assetGraphId}
/>
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we hadn't done this previously because this AssetNode contains links, and the DOM doesn't allow <a> tags within <a> tags :( At least, it was making React spew logs about invalid nesting.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmm interesting, I guess I could read e.metaKey and keep it in JS

Copy link
Member

@alangenfeld alangenfeld left a comment

Choose a reason for hiding this comment

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

just Link wrapper effectively right?

@salazarm salazarm force-pushed the marco/fe-333-cmd-clicking-on-an-asset-in-the-lineage-graph-should-open-a branch from b5d9cd5 to a58e6ce Compare May 21, 2024 13:57
@salazarm salazarm merged commit 4edbd60 into master May 21, 2024
1 of 2 checks passed
@salazarm salazarm deleted the marco/fe-333-cmd-clicking-on-an-asset-in-the-lineage-graph-should-open-a branch May 21, 2024 15:45
nikomancy pushed a commit that referenced this pull request May 22, 2024
…ew page (#21456)

## Summary & Motivation

Adding real links here to do this rather than using a JS click handler.




## How I Tested These Changes


https://github.com/dagster-io/dagster/assets/2286579/bee9eb20-cd0b-4f0f-8e8b-df2467ea5b23
danielgafni pushed a commit to danielgafni/dagster that referenced this pull request Jun 18, 2024
…ew page (dagster-io#21456)

## Summary & Motivation

Adding real links here to do this rather than using a JS click handler.




## How I Tested These Changes


https://github.com/dagster-io/dagster/assets/2286579/bee9eb20-cd0b-4f0f-8e8b-df2467ea5b23
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