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

Add hint for unknown node type #804

Merged
merged 1 commit into from
Feb 7, 2024
Merged

Add hint for unknown node type #804

merged 1 commit into from
Feb 7, 2024

Conversation

tplevko
Copy link
Contributor

@tplevko tplevko commented Feb 6, 2024

fix for #803

Screenshot from 2024-02-06 14-55-50

@lordrip
Copy link
Member

lordrip commented Feb 6, 2024

Just my 2 cents, I think we should show the information before the node definition, as it might be long and we could miss the explanation.

Also, should we use a yellowish color to convey some sort of warning?

Comment on lines 103 to 121
<Card>
<CardHeader>
<CardTitle>Node source</CardTitle>
</CardHeader>
<CardBody>
<CodeBlock>
<CodeBlockCode>{stringify(model)}</CodeBlockCode>
</CodeBlock>
</CardBody>
<CardFooter>
<Hint>
<HintTitle>Node is not a known type</HintTitle>
<HintBody>
Configuration properties are not available when the node is not a known type. Delete, or replace this
node with valid node.
</HintBody>
</Hint>
</CardFooter>
</Card>
Copy link
Member

Choose a reason for hiding this comment

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

I think this component is starting to grow too big to have here :), maybe it's time to move it to a dedicated file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure - moved to separate file

@lhein
Copy link
Contributor

lhein commented Feb 6, 2024

I'd rephrase the warning message a bit:

Unknow node type

The configuration for an unknown node cannot be changed in the configuration form. Please switch to the source code and correct the node type. Another option is to replace the step in the graphical editor but in that case you will probably lose the existing configuration.

@tplevko
Copy link
Contributor Author

tplevko commented Feb 7, 2024

Updated - moved to separate file, updated the Hint to Alert, with variant warning. Also changed the message.

With updates:
Screenshot from 2024-02-07 10-01-21

@lhein
Copy link
Contributor

lhein commented Feb 7, 2024

you did not change the headline, did you?

@tplevko
Copy link
Contributor Author

tplevko commented Feb 7, 2024

you did not change the headline, did you?

Missed that the headline was also changed - Updated.

Copy link
Member

@lordrip lordrip left a comment

Choose a reason for hiding this comment

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

This is a good enhancement @tplevko , thanks 💪

@lordrip lordrip merged commit d1c3b3b into KaotoIO:main Feb 7, 2024
9 checks passed
@lordrip lordrip linked an issue Feb 7, 2024 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unknown node - add help for the user
3 participants