-
Notifications
You must be signed in to change notification settings - Fork 112
Workflow Visualizer UX improvements #1367
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
Conversation
Signed-off-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
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.
Neat.
| val fields = Node::class.memberProperties | ||
| for (field in fields) { |
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.
oh nice. Ok i see you moved to reflection here. that's great. I think it makes sense.
This had no visible affect on UI performance
27f1fc9 to
5ee0c41
Compare
| .pointerInput(Unit) { | ||
| awaitEachGesture { | ||
| val event = awaitPointerEvent() | ||
| if (event.type == PointerEventType.Scroll) { | ||
| val scrollDeltaY = event.changes.first().scrollDelta.y | ||
| coroutineScope.launch { | ||
| lazyListState.scroll(MutatePriority.Default) { | ||
| scrollBy(scrollDeltaY * 10f) | ||
| } | ||
| } | ||
| } | ||
| } | ||
| }, |
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 there a particular reason you need this animation to outlive the pointerInput modifier's lifetime? It would be slightly cleaner to just wrap this with coroutineScope {}, i.e.
| .pointerInput(Unit) { | |
| awaitEachGesture { | |
| val event = awaitPointerEvent() | |
| if (event.type == PointerEventType.Scroll) { | |
| val scrollDeltaY = event.changes.first().scrollDelta.y | |
| coroutineScope.launch { | |
| lazyListState.scroll(MutatePriority.Default) { | |
| scrollBy(scrollDeltaY * 10f) | |
| } | |
| } | |
| } | |
| } | |
| }, | |
| .pointerInput(Unit) { | |
| coroutineScope { | |
| awaitEachGesture { | |
| val event = awaitPointerEvent() | |
| if (event.type == PointerEventType.Scroll) { | |
| val scrollDeltaY = event.changes.first().scrollDelta.y | |
| launch { | |
| lazyListState.scroll(MutatePriority.Default) { | |
| scrollBy(scrollDeltaY * 10f) | |
| } | |
| } | |
| } | |
| } | |
| } | |
| }, |
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.
Changed
| LaunchedEffect(expandedNodes) { | ||
| expandedNodes[node.id] = isAffected | ||
| } |
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 weird little bit of backwards data flow. Feels like there should be a presenter or something handling this?
It also looks like affectedNodes and expandedNodes effectively represent the same thing, since this code just synchronizes them, or partially synchronizes them? It's possible I'm misunderstanding the code.
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.
-
I'm not familiar with using a Presenter as a pattern, if you wouldn't mind explaining a potential solution (or point me to a resource). I can understand that UI should be using callbacks to affect data rather than directly mutating it, but when I experimented hoisting everything out of this Composable and into
RenderDiagram, the tree would no longer recompose everytime I open/closed a node.But I definitely could've been trying an incorrect way of doing it, leading it to fail.
-
Any
affectedNodesfor the specific renderpass will always be the same, but the nodes that are expanded can be changed. So we can open a non-affected node, and we can also close an affected node
b02a717 to
4f6ad22
Compare
4f6ad22 to
e9a03f6
Compare
Allow each node to be opened and closed to ignore irrelevant information for each frame. Nodes that were directly affected in the specific render pass, though, are all open by default, and everything else is closed by default.
Allow for using a mouse's vertical scroll to traverse through the row of frames.