Implement data sandboxing updates #20258
Conversation
salazarm
requested review from
bengotow,
hellendag,
jamiedemaria and
braunjj
and removed request for
jamiedemaria
|
Deploy preview for dagit-core-storybook ready! ✅ Preview Built with commit 21e5373. |
|
Deploy preview for dagit-storybook ready! ✅ Preview Built with commit 21e5373. |
salazarm
force-pushed
the
marco/fe-201-implement-data-sandboxing-ui-updates
branch
from
e6ae0a5 to
7f04891
Compare
There was a problem hiding this comment.
This all looks great! As someone unfamiliar with this workstream having a second set of "reasons" is kinda confusing and I wish there was an umbrella term for stale + changed that we could use where the stale components are now rendering both, but I can't think of anything good...
js_modules/dagster-ui/packages/ui-core/src/asset-data/AssetLiveDataProvider.tsx
Show resolved
Hide resolved
js_modules/dagster-ui/packages/ui-core/src/asset-graph/AssetNode.tsx
Outdated
Show resolved
Hide resolved
| {isNew ? ( | ||
| <NewInBranchTag changedReasons={liveData!.changedReasons!} assetKey={assetKey} /> | ||
| ) : null} |
There was a problem hiding this comment.
I wonder if we should rename StaleReasonsTag to something else -- it's a bit confusing that "stale reasons" are still a thing, but this StaleReasonsTag also displays "changed Reasons" which are a different thing...
I can't think of a good name though... maybe just <ReasonsTags />?
| if (!dependency) { | ||
| return <Caption>{` ${reason}`}</Caption>; | ||
| } | ||
| const content = useMemo(() => { |
There was a problem hiding this comment.
Not super important really but I don't know that this is particularly worth a memo, we do way worse in each render cycle 🙈 Though maybe we should be doing this!
There was a problem hiding this comment.
I separated the changed reason to a new component so I can remove this now! 😄
| style={{height: 24}} | ||
| > | ||
| {staleTag} | ||
| {isNew ? ( |
There was a problem hiding this comment.
Does this mean the change reason tag will only show if the asset is new? we'd also like to show for assets that aren't new but do have changes, the case when changedReasons = [ChangeReason.INPUTS] etc
| <BaseTag | ||
| fillColor={Colors.backgroundCyan()} | ||
| textColor={Colors.textCyan()} | ||
| label={changes.length ? 'Modified in branch' : 'Modified in branch'} |
There was a problem hiding this comment.
should one of these be New in branch? noticed that the new asset is now being marked "Modified in branch" instead of "New in branch"
## Summary & Motivation Implements updates to data sandboxing [more context](https://linear.app/dagster-labs/issue/FE-201/implement-data-sandboxing-ui-updates) ## How I Tested These Changes Asset graph:   Asset catalog: 
Summary & Motivation
Implements updates to data sandboxing more context
How I Tested These Changes
Asset graph:
Asset catalog:
