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

[ui] Fix “filter to group” action no longer working in the asset graph #23768

Merged
merged 1 commit into from
Aug 20, 2024

Conversation

bengotow
Copy link
Collaborator

@bengotow bengotow commented Aug 20, 2024

Summary & Motivation

https://linear.app/dagster-labs/issue/FE-521/[bug]-filter-to-group-action-no-longer-works-in-the-lineage-ui

The AssetGraphExplorer had an optional assetFilterState?: AssetFilterState that needed to be passed for this filtering to work, and it's no longer passed anywhere the component is used. I updated this to work the same way the clickable computeKind tags work.

How I Tested These Changes

I verified that the asset graph sidebar and asset graph collapsed + expanded nodes "Filter to group" options all still work

@bengotow bengotow requested review from salazarm and hellendag August 20, 2024 19:55
@@ -74,8 +74,6 @@ type Props = {
options: GraphExplorerOptions;
setOptions?: (options: GraphExplorerOptions) => void;

assetFilterState?: AssetFilterState;
Copy link
Contributor

Choose a reason for hiding this comment

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

oh could we just make this non optional? I think the prop not being passed in is probably a bug

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Discussed in Slack, it looks like the only scoping that happens above this component is passed in via fetchOptions={{pipelineSelector}}(on job pages) and all the other filter state is now within the AssetGraphExplorer, so it's ok to rm this!

Copy link

github-actions bot commented Aug 20, 2024

Deploy preview for dagit-core-storybook ready!

✅ Preview
https://dagit-core-storybook-bczzsty22-elementl.vercel.app
https://bengotow-2024-08-FE-521.core-storybook.dagster-docs.io

Built with commit da173f0.
This pull request is being automatically deployed with vercel-action

Copy link
Contributor

@salazarm salazarm left a comment

Choose a reason for hiding this comment

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

thanks for looking into this!

@bengotow bengotow force-pushed the bengotow-2024-08/FE-521 branch from 1c77312 to 62a54ac Compare August 20, 2024 20:44
@bengotow bengotow force-pushed the bengotow-2024-08/FE-521 branch from 62a54ac to da173f0 Compare August 20, 2024 21:52
@bengotow bengotow merged commit dea312f into master Aug 20, 2024
2 checks passed
@bengotow bengotow deleted the bengotow-2024-08/FE-521 branch August 20, 2024 22:01
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