-
Notifications
You must be signed in to change notification settings - Fork 5
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
Prevent asterism updates for executed observations, add undo/redo #4056
Conversation
BundleMonFiles updated (1)
Unchanged files (7)
Total files change +1.47KB +0.06% Final result: ✅ View report in BundleMon website ➡️ |
20038dd
to
807e16f
Compare
@@ -20,4 +20,25 @@ object AsterismQueriesGQL { | |||
} | |||
""" | |||
} | |||
|
|||
// Group these 2 mutations together so that the targetEdit and observationEdit subscriptions come |
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.
Is this guaranteed to work?
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.
Well, we reduce the transit time involved, but they still aren't real close together. That timing depends on when the database updates happen for the 2 mutations, I guess. Still seemed better than doing them individually. 🤷♂️
@@ -59,6 +59,7 @@ object AsterismEditorTile: | |||
backButton: Option[VdomNode] = none | |||
)(using FetchClient[IO, ObservationDB], Logger[IO]): Tile = { | |||
// Save the time here. this works for the obs and target tabs | |||
// It's OK to save the viz time for executed observations, I think. | |||
val vizTimeView = | |||
vizTime.withOnMod(t => ObsQueries.updateVisualizationTime[IO](obsIds.toList, t).runAsync) |
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 needs an update 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.
Actually, it looks like it is still called ObsQueries.updateVisualizationTime
in your PR. 😄 So, I don't think this needs to change. Unless there is another reason.
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.
lazy cquiroz
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 quite an undertaking! thanks
LGTM
807e16f
to
6fa38f7
Compare
No description provided.