-
-
Notifications
You must be signed in to change notification settings - Fork 341
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
Fix function editor save #1507
Fix function editor save #1507
Conversation
@@ -106,6 +106,9 @@ export default function QueryNodeEditorDialog<Q>({ | |||
const node = useLatest(nodeProp); | |||
|
|||
const [input, setInput] = React.useState<appDom.QueryNode<Q>>(node); | |||
React.useEffect(() => { |
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 repeated, there's the same effect in line 237 (I might've added in the last PR I merged).
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.
Ok, I'm removing the other one to keep it closer to where the state is created. It's a good convention to keep these lines together when the behavior is desired of reinitializing when initial value changes.
|
||
await expect(editorModel.appCanvas.getByText('edited hello')).toBeVisible(); | ||
|
||
await editorModel.componentEditor.getByRole('button', { name: 'edited' }).click(); |
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.
When we add more tests for the query editor it might make sense for some of these to have methods/properties we can reuse.
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.
Looks good, just need to remove the duplicate effect I think.
Closes #1452
Turns out the shortcut was working after all. The problem was that the update didn't reflect in the UI afterwards