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

[2/n][Asset Graph Sidebar] - Consolidate filtering #16536

Merged
merged 2 commits into from
Sep 20, 2023
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -309,10 +309,7 @@ const AssetGraphExplorerWithData: React.FC<WithDataProps> = ({
{graphQueryItems.length === 0 ? (
<EmptyDAGNotice nodeType="asset" isGraph />
) : applyingEmptyDefault ? (
<LargeDAGNotice
nodeType="asset"
anchorLeft={fetchOptionFilters ? '300px' : '40px'}
/>
<LargeDAGNotice nodeType="asset" anchorLeft="40px" />
) : Object.keys(assetGraphData.nodes).length === 0 ? (
<EntirelyFilteredDAGNotice nodeType="asset" />
) : undefined}
Expand Down Expand Up @@ -435,10 +432,7 @@ const AssetGraphExplorerWithData: React.FC<WithDataProps> = ({
</OptionsOverlay>
)}

<Box
flex={{direction: 'column', alignItems: 'flex-end', gap: 8}}
style={{position: 'absolute', right: 12, top: 8}}
>
<Box style={{position: 'absolute', right: 12, top: 8}}>
<Box flex={{alignItems: 'center', gap: 12}}>
<QueryRefreshCountdown
refreshState={liveDataRefreshState}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,114 @@
import {Box, Icon} from '@dagster-io/ui-components';
import React from 'react';

import {AssetGroupSelector} from '../graphql/types';
import {TruncatedTextWithFullTextOnHover} from '../nav/getLeftNavItemsForOption';
import {useFilters} from '../ui/Filters';
import {FilterObject} from '../ui/Filters/useFilter';
import {useStaticSetFilter} from '../ui/Filters/useStaticSetFilter';
import {DagsterRepoOption, WorkspaceContext} from '../workspace/WorkspaceContext';
import {buildRepoAddress, buildRepoPathForHuman} from '../workspace/buildRepoAddress';

export const AssetGraphExplorerFilters = React.memo(
({
assetGroups,
visibleAssetGroups,
setGroupFilters,
}:
| {
assetGroups: AssetGroupSelector[];
visibleAssetGroups: AssetGroupSelector[];
setGroupFilters: (groups: AssetGroupSelector[]) => void;
}
| {assetGroups?: null; setGroupFilters?: null; visibleAssetGroups?: null}) => {
const {allRepos, visibleRepos, toggleVisible} = React.useContext(WorkspaceContext);

const visibleReposSet = React.useMemo(() => new Set(visibleRepos), [visibleRepos]);

const reposFilter = useStaticSetFilter<DagsterRepoOption>({
name: 'Repository',
icon: 'repo',
allValues: allRepos.map((repo) => ({
key: repo.repository.id,
value: repo,
match: [buildRepoPathForHuman(repo.repository.name, repo.repositoryLocation.name)],
})),
menuWidth: '300px',
renderLabel: ({value}) => (
<Box flex={{direction: 'row', gap: 4, alignItems: 'center'}}>
<Icon name="repo" />
<TruncatedTextWithFullTextOnHover
text={buildRepoPathForHuman(value.repository.name, value.repositoryLocation.name)}
/>
</Box>
),
getStringValue: (value) =>
buildRepoPathForHuman(value.repository.name, value.repositoryLocation.name),
initialState: visibleReposSet,
onStateChanged: (values) => {
allRepos.forEach((repo) => {
if (visibleReposSet.has(repo) !== values.has(repo)) {
toggleVisible([buildRepoAddress(repo.repository.name, repo.repositoryLocation.name)]);
}
});
},
});

const groupsFilter = useStaticSetFilter<AssetGroupSelector>({
name: 'Asset Groups',
icon: 'asset_group',
allValues: (assetGroups || []).map((group) => ({
key: group.groupName,
value:
visibleAssetGroups?.find(
(visibleGroup) =>
visibleGroup.groupName === group.groupName &&
visibleGroup.repositoryName === group.repositoryName &&
visibleGroup.repositoryLocationName === group.repositoryLocationName,
) ?? group,
match: [group.groupName],
})),
menuWidth: '300px',
renderLabel: ({value}) => (
<Box flex={{direction: 'row', gap: 4, alignItems: 'center'}}>
<Icon name="repo" />
<TruncatedTextWithFullTextOnHover
tooltipText={
value.groupName +
' - ' +
buildRepoPathForHuman(value.repositoryName, value.repositoryLocationName)
}
text={
<>
{value.groupName}
<span style={{opacity: 0.5, paddingLeft: '4px'}}>
{buildRepoPathForHuman(value.repositoryName, value.repositoryLocationName)}
</span>
</>
}
/>
</Box>
),
getStringValue: (group) => group.groupName,
initialState: React.useMemo(() => new Set(visibleAssetGroups ?? []), [visibleAssetGroups]),
onStateChanged: (values) => {
if (setGroupFilters) {
setGroupFilters(Array.from(values));
}
},
});

const filters: FilterObject[] = [];
if (allRepos.length > 1) {
filters.push(reposFilter);
}
if (assetGroups) {
filters.push(groupsFilter);
}
const {button} = useFilters({filters});
if (allRepos.length <= 1 && !assetGroups) {
return null;
}
return button;
},
);
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,15 @@ export const AssetGraphExplorerSidebar = React.memo(
const rootNodes = React.useMemo(
() =>
Object.keys(assetGraphData.nodes)
.filter((id) => !assetGraphData.upstream[id])
.filter(
(id) =>
// When we filter to a subgraph, the nodes at the root aren't real roots, but since
// their upstream graph is cutoff we want to show them as roots in the sidebar.
// Find these nodes by filtering on whether there parent nodes are in assetGraphData
!Object.keys(assetGraphData.upstream[id] ?? {}).filter(
(id) => assetGraphData.nodes[id],
).length,
)
.sort((a, b) =>
COLLATOR.compare(
getDisplayName(assetGraphData.nodes[a]!),
Expand All @@ -57,14 +65,12 @@ export const AssetGraphExplorerSidebar = React.memo(
const node = queue.shift()!;
renderedNodes.push(node);
if (openNodes.has(node.path)) {
const downstream = Object.keys(assetGraphData.downstream[node.id] || {});
if (downstream) {
queue.unshift(
...downstream
.filter((id) => assetGraphData.nodes[id])
.map((id) => ({level: node.level + 1, id, path: `${node.path}:${id}`})),
);
}
const downstream = Object.keys(assetGraphData.downstream[node.id] || {}).filter(
(id) => assetGraphData.nodes[id],
);
queue.unshift(
...downstream.map((id) => ({level: node.level + 1, id, path: `${node.path}:${id}`})),
);
}
}
return renderedNodes;
Expand All @@ -89,6 +95,9 @@ export const AssetGraphExplorerSidebar = React.memo(
let currentId = lastSelectedNode.id;
let next: string | undefined;
while ((next = Object.keys(assetGraphData.upstream[currentId] ?? {})[0])) {
if (!assetGraphData.nodes[next]) {
break;
}
path = `${next}:${path}`;
currentId = next;
}
Expand All @@ -108,7 +117,7 @@ export const AssetGraphExplorerSidebar = React.memo(
});
}
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [lastSelectedNode]);
}, [lastSelectedNode, assetGraphData]);

const indexOfLastSelectedNode = React.useMemo(
() =>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,17 +4,17 @@ import * as React from 'react';
import {useHistory, useParams} from 'react-router-dom';

import {AssetGraphExplorer} from '../asset-graph/AssetGraphExplorer';
import {AssetGraphExplorerFilters} from '../asset-graph/AssetGraphExplorerFilters';
import {AssetGraphFetchScope} from '../asset-graph/useAssetGraphData';
import {AssetLocation} from '../asset-graph/useFindAssetLocation';
import {AssetGroupSelector} from '../graphql/types';
import {useDocumentTitle} from '../hooks/useDocumentTitle';
import {useQueryPersistedState} from '../hooks/useQueryPersistedState';
import {RepoFilterButton} from '../instance/RepoFilterButton';
import {ExplorerPath} from '../pipelines/PipelinePathUtils';
import {ReloadAllButton} from '../workspace/ReloadAllButton';
import {WorkspaceContext} from '../workspace/WorkspaceContext';

import {AssetGroupSuggest, buildAssetGroupSelector} from './AssetGroupSuggest';
import {buildAssetGroupSelector} from './AssetGroupSuggest';
import {assetDetailsPathForKey} from './assetDetailsPathForKey';
import {
globalAssetGraphPathFromString,
Expand All @@ -27,7 +27,7 @@ interface AssetGroupRootParams {

export const AssetsGroupsGlobalGraphRoot: React.FC = () => {
const {0: path} = useParams<AssetGroupRootParams>();
const {allRepos, visibleRepos} = React.useContext(WorkspaceContext);
const {visibleRepos} = React.useContext(WorkspaceContext);
const history = useHistory();

const [filters, setFilters] = useQueryPersistedState<{groups: AssetGroupSelector[]}>({
Expand Down Expand Up @@ -103,14 +103,14 @@ export const AssetsGroupsGlobalGraphRoot: React.FC = () => {
<AssetGraphExplorer
fetchOptions={fetchOptions}
fetchOptionFilters={
<>
{allRepos.length > 1 && <RepoFilterButton />}
<AssetGroupSuggest
assetGroups={assetGroups}
value={filters.groups || []}
onChange={(groups) => setFilters({...filters, groups})}
/>
</>
<AssetGraphExplorerFilters
assetGroups={assetGroups}
visibleAssetGroups={React.useMemo(() => filters.groups || [], [filters.groups])}
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 the decode in the useQueryPersistedState on line 35 achieves this and you don't need the memo here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah okay wasn't sure

setGroupFilters={React.useCallback((groups) => setFilters({...filters, groups}), [
Copy link
Collaborator

Choose a reason for hiding this comment

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

Still not a fan of inlining hooks into the rendered content because it can break with early returns / refactoring (eg: if you moved this into const content = () => {...) but maybe I will get used to it 😂

Copy link
Contributor Author

@salazarm salazarm Sep 18, 2023

Choose a reason for hiding this comment

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

I like inlining because of code co-colocation but yeah I can definitely see refactoring being a bit more work although not very challenging work

filters,
setFilters,
])}
/>
}
options={{preferAssetRendering: true, explodeComposites: true}}
explorerPath={globalAssetGraphPathFromString(path)}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -140,11 +140,18 @@ const TruncatingName = styled.div`

export const TruncatedTextWithFullTextOnHover = React.forwardRef(
(
{text, tooltipStyle, ...rest}: {text: string; tooltipStyle?: string},
{
text,
tooltipStyle,
tooltipText,
...rest
}:
| {text: string; tooltipStyle?: string; tooltipText?: null}
| {text: React.ReactNode; tooltipStyle?: string; tooltipText: string},
ref: React.ForwardedRef<HTMLDivElement>,
) => (
<TruncatingName
data-tooltip={text}
data-tooltip={tooltipText ?? text}
data-tooltip-style={tooltipStyle ?? LabelTooltipStyles}
ref={ref}
{...rest}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -250,7 +250,7 @@ export const FilterDropdown = ({filters, setIsOpen, setPortaledElements}: Filter
<Container
ref={parentRef}
style={{
maxHeight: '500px',
maxHeight: `min(500px, 50vh)`,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Stupid question I think but does min work outside of calc? I thought this would have to be calc(min(500px, 50vh))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes it does

overflowY: 'auto',
width: selectedFilter?.menuWidth || 'auto',
}}
Expand Down