[1/2] AMP Timeline scaffold + Tick Timeline component #17046
Conversation
|
Deploy preview for dagit-core-storybook ready! ✅ Preview Built with commit 1ceb3ac. |
js_modules/dagster-ui/packages/ui-core/src/app/CloudFeatureFlag.tsx
Outdated
Show resolved
Hide resolved
salazarm
force-pushed
the
salazarm/amp-timeline
branch
from
b6892cc to
c6be0af
Compare
salazarm
changed the title
js_modules/dagster-ui/packages/ui-core/src/instigation/LiveTickTimeline2.tsx
Show resolved
Hide resolved
js_modules/dagster-ui/packages/ui-core/src/app/time/Timestamp.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
I notice that the content alignment in the dialog rows is a little bit off, and not vertically centered. I think the dialog could also be a bit wider -- especially since the group name will also need to be truncated pretty often.
Requesting changes mostly for the date-time thing, which I think can be discussed separately.
...-ui/packages/ui-core/src/assets/auto-materialization/AutomaterializationTickDetailDialog.tsx
Show resolved
Hide resolved
...-ui/packages/ui-core/src/assets/auto-materialization/AutomaterializationTickDetailDialog.tsx
Show resolved
Hide resolved
js_modules/dagster-ui/packages/ui-core/src/instigation/LiveTickTimeline2.tsx
Outdated
Show resolved
Hide resolved
js_modules/dagster-ui/packages/ui-core/src/instigation/LiveTickTimeline2.tsx
Outdated
Show resolved
Hide resolved
js_modules/dagster-ui/packages/ui-core/src/instigation/LiveTickTimeline2.tsx
Outdated
Show resolved
Hide resolved
| </Box> | ||
| {!queryResult.data ? ( | ||
| <Box padding={{vertical: 48}}> | ||
| <Spinner purpose="page" /> |
There was a problem hiding this comment.
I'm such a hater of the page spinner
There was a problem hiding this comment.
we need a page shimmer
...ules/dagster-ui/packages/ui-core/src/assets/auto-materialization/AutomaterializationRoot.tsx
Outdated
Show resolved
Hide resolved
...ckages/ui-core/src/assets/auto-materialization/AutomaterializationEvaluationHistoryTable.tsx
Outdated
Show resolved
Hide resolved
...-ui/packages/ui-core/src/assets/auto-materialization/AutomaterializationTickDetailDialog.tsx
Outdated
Show resolved
Hide resolved
...ules/dagster-ui/packages/ui-core/src/assets/auto-materialization/AutomaterializationRoot.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
I haven't read through this PR yet, but wanted to chime in with a general comment that may or may not be relevant here: ideally we would end up in a state where, when we make a change to either the sensor page or the auto-materialize page, we're forced to consider the implications to the other one (vs. needing to remember to duplicate the change across two places).
|
Deploy preview for dagit-storybook ready! ✅ Preview Built with commit 1ceb3ac. |
salazarm
force-pushed
the
salazarm/amp-timeline
branch
from
660ad2a to
cc70012
Compare
| export function ifPlural(count: number | undefined | null, string: string) { | ||
| return count === 1 ? '' : string; | ||
| } |
There was a problem hiding this comment.
This is okay, though it might not really help if the verb disagrees. I tend to just go with writing out the entire string, since I find it easier to comprehend the copy if I don't have to mentally add an s (or whatever the declension is) to the noun:
count === 1 ? '1 run requested' : `${count} runs requested`
Doesn't matter a ton for now anyway, as long as we're providing appropriate strings, until the point in the future when we decide to internationalize the app.
There was a problem hiding this comment.
Ah yeah I guess I should make you pass in both versions of the string instead
| const MINUTE = 60 * 1000; | ||
| const THREE_MINUTES = 3 * MINUTE; | ||
| const FIVE_MINUTES = 5 * MINUTE; | ||
| const TWENTY_MINUTES = 20 * MINUTE; |
Summary & Motivation
Scaffold for AMP Timeline, feature gated with a context that Cloud will use to enable the feature.
How I Tested These Changes
locally + shadow dagit