[Asset graph] Add query refresh button with "Date at most X seconds old" tooltip #16886
Conversation
|
Deploy preview for dagit-storybook ready! ✅ Preview Built with commit 51d97c2. |
|
Deploy preview for dagit-core-storybook ready! ✅ Preview Built with commit 51d97c2. |
salazarm
force-pushed
the
salazarm/asset-graph-query-refresh
branch
from
820e799 to
06457c8
Compare
salazarm
changed the title
| <Button | ||
| icon={<Icon name="refresh" color={Colors.Gray400} />} | ||
| onClick={() => { |
There was a problem hiding this comment.
Not a huge fan of this looking different than all our other refresh buttons but I'd defer to josh on this one
There was a problem hiding this comment.
Yeah this was josh's design, we thought the text makes the button too long
| const [text, setText] = React.useState(dayjs(timestamp).fromNow(true)); | ||
| React.useEffect(() => { | ||
| const interval = setInterval(() => { | ||
| setText(dayjs(timestamp).fromNow(true)); |
There was a problem hiding this comment.
Hmm, is this too flickery to display this outside a tooltip? Would be nice to have data age onscreen in some way vs just a refresh button. Would "At most 22 sec old" in gray text be too long? Feels like it'd match our refreshableCountdown UI a bit more
There was a problem hiding this comment.
Nick had some qualms with the countdown counting upwards, he said it gave him anxiety
| const [isGloballyRefreshing, setIsGloballyRefreshing] = React.useState(false); | ||
| const [oldestDataTimestamp, setOldestDataTimestamp] = React.useState(0); | ||
|
|
There was a problem hiding this comment.
It feels a bit odd that oldestDataTimestamp is a separate piece of state when it's a pure derivation of allAssetKeys and cache? (eg: given a set of asset keys and the cache, I should be able to tell you the oldest data timestamp)
It might be nice to write it as a useMemo instead of imperatively updating it in all the correct places where you call fetchData. It seems like it'd be super easy to forget it in one of those callsites and have this state fall out of sync.
There was a problem hiding this comment.
it depends on lastFetchedOrRequested which is a global so a useMemo wouldn't work here. lastFetchedOrRequested is only mutated by one function _batchedAssetsFetch which is the same function that calls onUpdatedOrUpdating() so it's all centralized.
js_modules/dagster-ui/packages/ui-core/src/asset-data/AssetLiveDataProvider.tsx
Show resolved
Hide resolved
## Summary & Motivation
Requests from Josh:
- [x] Can we fix the alignment of the Selection box query row? Right now
it does not align with the items in the sidebar (designs below)
- [x] Put all items into a toolbar that matches the sidebar header
- [x] Stretch the query input to fill space
- [x] Add an info icon action that opens a dialog (see designs)
- [x] Add a “Clear query” action to make it easier to reset this field
## How I Tested These Changes
locally:
<img width="1496" alt="Screenshot 2023-09-29 at 2 57 04 AM"
src="https://github.com/dagster-io/dagster/assets/2286579/ff368fcf-f47e-42f7-80ab-64beec16ea98">
<img width="1496" alt="Screenshot 2023-09-29 at 2 46 53 AM"
src="https://github.com/dagster-io/dagster/assets/2286579/e5f0f945-bbc1-4782-a9d7-75fef52fd7a2">
<img width="869" alt="Screenshot 2023-09-29 at 2 46 44 AM"
src="https://github.com/dagster-io/dagster/assets/2286579/54f352ea-ba8a-4ab8-908c-b0841bea2aec">
Summary & Motivation
Adding back the ability to refresh data and also including a message indicating how old the last visible refreshed data on the screen is.
How I Tested These Changes