-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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] Refactoring to support asset health overview #21081
Conversation
const [menuPosition, setMenuPosition] = React.useState<{top: number; left: number}>({ | ||
top: 0, | ||
left: 0, | ||
const [menuPosition, setMenuPosition] = React.useState<{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a fix for right-clicking on an asset on the far right edge of the screen - the menu needs to be anchored from the other side so it's onscreen
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ooo I figured blueprint would have logic to make sure it's always visible regardless of setting, that's a bummer that it doesn't
Deploy preview for dagit-storybook ready! ✅ Preview Built with commit 3019fcc. |
Deploy preview for dagit-core-storybook ready! ✅ Preview Built with commit 3019fcc. |
js_modules/dagster-ui/packages/ui-core/src/asset-graph/sidebar/AssetSidebarNode.tsx
Outdated
Show resolved
Hide resolved
}); | ||
|
||
const showMenu = (e: React.MouseEvent) => { | ||
const anchor = window.innerWidth - e.pageX < 240 ? 'right' : 'left'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
instead of hardcoding 240 here should we take it as a prop with maybe 240 as the default? We could also use useViewport
to get the real width I think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm that's a good point, we probably should calculate this at runtime rather than hardcoding to 240 -- we'll need to do the first render invisibly I think to avoid it appearing left anchored and then jumping to the right anchor but I think we can do that 👀
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
edit hrm, it looks like we have to render hidden, size, render visible to do this with the correct breakpoint, going to come back to it in a follow-up!
ef270bc
to
e8ac396
Compare
js_modules/dagster-ui/packages/ui-core/src/assets/useAssetCatalogFiltering.tsx
Show resolved
Hide resolved
js_modules/dagster-ui/packages/ui-core/src/assets/useAssetCatalogFiltering.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, I think the asset graph is still defining it's own filters after this right?
306b1ed
to
dd385af
Compare
…2024-04/asset-health-overview # Conflicts: # js_modules/dagster-ui/packages/ui-core/src/asset-graph/sidebar/StatusDot.tsx # js_modules/dagster-ui/packages/ui-core/src/assets/AssetsCatalogTable.tsx # js_modules/dagster-ui/packages/ui-core/src/assets/asset-checks/AssetCheckStatusTag.tsx # js_modules/dagster-ui/packages/ui-core/src/insights/InsightsLineChart.tsx # js_modules/dagster-ui/packages/ui-core/src/overview/OverviewTabs.tsx # js_modules/dagster-ui/packages/ui-core/src/overview/OverviewTimelineRoot.tsx
## Summary & Motivation This PR extracts the fetching and filtering portions of the asset catalog into a high-level hook so that it's easier to build UI that re-uses the asset catalog's index caching and filtering. I also exported a few pieces of the left asset sidebar and updated asset context menus to allow them to appear in more places. ## How I Tested These Changes Tested this as part of the new asset health feature in cloud --------- Co-authored-by: bengotow <[email protected]>
Summary & Motivation
This PR extracts the fetching and filtering portions of the asset catalog into a high-level hook so that it's easier to build UI that re-uses the asset catalog's index caching and filtering. I also exported a few pieces of the left asset sidebar and updated asset context menus to allow them to appear in more places.
How I Tested These Changes
Tested this as part of the new asset health feature in cloud