Fix component browser interactions. #8303
Conversation
MichaelMauderer
added
the
CI: No changelog needed
| applySuggestion() | ||
| acceptInput() |
There was a problem hiding this comment.
| applySuggestion() | |
| acceptInput() | |
| applySuggestion() |
Applying accepting suggestion should also accept input, unless we want to enter module, in which case CB should not close.
Also, I would rename it as acceptSuggestion, and acceptInputRaw could be just acceptInput.
There was a problem hiding this comment.
The applySuggestion method, right now, adds the currently selected suggestion in the CB to the input. This is what happens when pressing tab.
I thought we had three interactions here:
tabonly applies the selected suggestion but keeps the CB openenterchooses the applied the selected suggestion and closes the CBctrl+entercloses the CB with the current input, without applying the selected suggestion.
There was a problem hiding this comment.
Sorry, I re-read my comment, and now I cannot say what have I had in mind. You are right, of course. Only the names:
tabI usually name "applying suggestion"enteris "accepting suggestion"ctrl+enteris "accepting input"
There was a problem hiding this comment.
That sounds good, I'll apply that naming to the code, add a doc and that should make it consistent and clear.
| gap: gapBetweenNodes, | ||
| }).position | ||
| } else { | ||
| return mouseDictatedPlacement(nodeSize, placementEnvironment.value).position |
There was a problem hiding this comment.
i'm testing this out during the bookclub and this doesn't work, since this value is continuously computed.
specifically, doing this makes the cb follow the mouse - which is kinda cool to see, but that makes it not very useful, as it would be no longer possible to click around in the CB entries, docs breadcrumbs, and the CB input.
There was a problem hiding this comment.
i don't think this needs to be changed, but you'd need another ref
const realComponentBrowserPosition = ref<Vec2>()
watch(componentBrowserVisible, visible => {
if (visible) realComponentBrowserPosition.value = componentBrowserPosition.value
else realComponentBrowserPosition.value = undefined
})There was a problem hiding this comment.
I, in turn, would just make function "openComponentBrowserParams" which inter alia will just set computed position to componentBrowserPosition without using reactivity, and then set visible to true.
But if you stick to @somebody1234 solution for some reason, I prefer renaming componentBrowserPosition -> openedComponentBrowserPosition or newComponentBrowserPosition and realComponentBrowserPosition -> just componentBrowserPosition.
There was a problem hiding this comment.
I went with refactoring it to a function targetComponentBrowserPosition which is used to update the componentBrowserPosition when required.
…emporary-Node-During-Node-Crteation # Conflicts: # app/gui2/src/components/ComponentBrowser/__tests__/placement.test.ts # app/gui2/src/components/GraphEditor.vue
…emporary-Node-During-Node-Crteation # Conflicts: # app/gui2/src/components/GraphEditor.vue
|
@Frizi Just missing your review now. |
mergify
bot
deleted the
wip/MichaelMauderer/gui2/Avoid-Creating-Temporary-Node-During-Node-Crteation
branch
| const editedInfo = graphStore.editedNodeInfo | ||
| const isEditingNode = editedInfo != null | ||
| const hasNodeSelected = nodeSelection.selected.size > 0 | ||
| const nodeSize = new Vec2(0, 24) |
There was a problem hiding this comment.
I'm not sure I understand this hardcoded value. This is the expected size of a node that is about to be created by the component browser? If so, it should be loaded from a predefined constant. Let's change that soon, but in separate PR to not block anything.
| const targetPos = targetNode?.position ?? Vec2.Zero | ||
| return targetPos.add(COMPONENT_BROWSER_TO_NODE_OFFSET) | ||
| } else if (hasNodeSelected) { | ||
| const gapBetweenNodes = 48.0 |
There was a problem hiding this comment.
Another thing that definitely should be a constant. Also, why isn't the default gap value used instead?
Pull Request Description
Fixes: #8064
Fixes broken interactions with of the component browser.
Checklist
Please ensure that the following checklist has been satisfied before submitting the PR:
Scala,
Java,
and
Rust
style guides. In case you are using a language not listed above, follow the Rust style guide.
./run ide build.