[2/n][Asset Graph Sidebar] - Consolidate filtering #16536
Conversation
|
Deploy preview for dagit-storybook ready! ✅ Preview Built with commit a47a16c. |
|
Deploy preview for dagit-core-storybook ready! ✅ Preview Built with commit 37b3e58. |
There was a problem hiding this comment.
This looks better but I'm worried that one of the primary use cases was /hiding/ one or two repos or asset groups and that is now more challenging because you have to check every single box? I think we should talk to Josh about this one and see if it's desirable without the ability to say "NOT my_massive_asset_group"
| @@ -250,7 +250,7 @@ export const FilterDropdown = ({filters, setIsOpen, setPortaledElements}: Filter | |||
| <Container | |||
| ref={parentRef} | |||
| style={{ | |||
| maxHeight: '500px', | |||
| maxHeight: `min(500px, 50vh)`, | |||
There was a problem hiding this comment.
Stupid question I think but does min work outside of calc? I thought this would have to be calc(min(500px, 50vh))
| </> | ||
| <AssetGraphExplorerFilters | ||
| assetGroups={assetGroups} | ||
| visibleAssetGroups={React.useMemo(() => filters.groups || [], [filters.groups])} |
There was a problem hiding this comment.
I think the decode in the useQueryPersistedState on line 35 achieves this and you don't need the memo here?
There was a problem hiding this comment.
ah okay wasn't sure
| <AssetGraphExplorerFilters | ||
| assetGroups={assetGroups} | ||
| visibleAssetGroups={React.useMemo(() => filters.groups || [], [filters.groups])} | ||
| setGroupFilters={React.useCallback((groups) => setFilters({...filters, groups}), [ |
There was a problem hiding this comment.
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 😂
There was a problem hiding this comment.
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
The existing behavior is still intact, this was mostly a visual change |
salazarm
force-pushed
the
salazarm/asset-sidebar-2
branch
from
a47a16c to
8eb59a1
Compare
…16537) ## Summary & Motivation Hooks up showing upstream/downstream graph to the asset selection filter ## How I Tested These Changes Used the feature with the toys repo. <img width="315" alt="Screenshot 2023-09-15 at 1 55 06 AM" src="https://github.com/dagster-io/dagster/assets/2286579/52d79d84-0de0-48ee-af92-83267dc109e2"> <img width="244" alt="Screenshot 2023-09-15 at 1 55 02 AM" src="https://github.com/dagster-io/dagster/assets/2286579/1bd509fb-171b-4745-b32f-4b0a9e5135ea">
Summary & Motivation
Consolidate the group and code location filters to use our Filtering component.
How I Tested These Changes