Skip to content
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

CB self icons #10006

Merged
merged 5 commits into from
May 21, 2024
Merged

CB self icons #10006

merged 5 commits into from
May 21, 2024

Conversation

kazcw
Copy link
Contributor

@kazcw kazcw commented May 20, 2024

Pull Request Description

In the component browser input field, render the source node as an icon instead of its identifier; an edge connects the icon to the source node's output port.

Screen.Recording.2024-05-20.at.10.49.36.mov

Closes #9210.

Important Notes

  • Fix node selection being visible (but glitched) while editing node.
  • Fix bug in CB positioning when editing a node at non-default zoom.
  • Fix disconnected edge hover allowing self-connection.
  • Consolidate some color logic in nodeColors.

Checklist

Please ensure that the following checklist has been satisfied before submitting the PR:

  • The documentation has been updated, if necessary.
  • Screenshots/screencasts have been attached, if there are any visual changes. For interactive or animated visual changes, a screencast is preferred.
  • All code follows the
    Scala,
    Java,
    TypeScript,
    and
    Rust
    style guides. In case you are using a language not listed above, follow the Rust style guide.
  • Unit tests have been written where possible.

- Fix node selection being visible (but glitched) while editing node.
- Fix bug in CB positioning when editing a node at non-default zoom.
- Fix disconnected edge hover allowing self-connection.
- Consolidate some color logic in `nodeColors`
@kazcw kazcw self-assigned this May 20, 2024
@kazcw kazcw added the CI: No changelog needed Do not require a changelog entry for this PR. label May 20, 2024
Comment on lines -102 to -103
// When user, right after opening CB with source node types operator, we should
// re-initialize input with it instead of dot at the end.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we keep this comment? It may be helpful, as what we do here is quite unusual.

const imports = ref<RequiredImport[]>([])
const processingAIPrompt = ref(false)
const toastError = useToast.error()
const sourceNode = ref<string>()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an identifier, I guess? Then please put that in the name (string could be theoretically ExpressionId, node's expression or whatever).

Comment on lines 422 to 428
if (ctx.type === 'insert' && ctx.position === 0 && sourceNode.value) return entry.name
if (
ctx.type === 'changeIdentifier' &&
ctx.identifier.inner.whitespaceStartInCodeParsed === 0 &&
sourceNode.value
)
return entry.name
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we could have fewer exceptions of this kind, if we use a fullAst computed value, i.e. parsed ${sourceNode}.${text}. Then in places where we're returning new input, we could just skip ${sourceNode}. prefix. As a bonus, the fullAst could be published and used to create a new node.

But I do not treat it as a blocker, as it might be harder than I think. Or not actually cleaner.

Comment on lines 41 to 42
() =>
graph.unconnectedEdge?.anchor.type === 'mouse' &&
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps instead of unconnectedEdge we could have two fields: editedEdge and componentBrowserEdge of more specific type - here, for example, you would not have to check anchor type.

@farmaazon
Copy link
Contributor

An additional note: if I understand this implementation, when someone will open CB without source node and type operator1., the self argument will be replaced with icon; which is nice, except that the only way of removing it is... writing an operator, what is far from intuitive. I think it's better to display the icon only when there was an input node specified upon CB creation, and not restore it otherwise.

But this is a rare case for rather advanced users, so let's discuss it after merging.

@kazcw kazcw requested a review from AdRiley as a code owner May 21, 2024 17:37
@kazcw kazcw added the CI: Ready to merge This PR is eligible for automatic merge label May 21, 2024
@mergify mergify bot merged commit 1633965 into develop May 21, 2024
35 checks passed
@mergify mergify bot deleted the wip/kw/cb-icons branch May 21, 2024 18:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI: No changelog needed Do not require a changelog entry for this PR. CI: Ready to merge This PR is eligible for automatic merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Component Browser should not be prepopulated with text and should be connected
2 participants