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

Fix changing tabs closing query editor #1784

Merged
merged 13 commits into from
Mar 24, 2023

Conversation

apedroferreira
Copy link
Member

@apedroferreira apedroferreira commented Mar 22, 2023

Closes #1742

  • Fix query editor closing when switching browser tabs
  • Also fixes console error when opening the query editor due to margin: auto in the page node.
Screen.Recording.2023-03-22.at.20.03.43.mov

@apedroferreira apedroferreira added the bug 🐛 Something doesn't work label Mar 22, 2023
@apedroferreira apedroferreira self-assigned this Mar 22, 2023
@@ -130,8 +130,12 @@ export default function QueryEditor() {
const { queries = [] } = appDom.getChildNodes(dom, page) ?? [];

const handleEditStateDialogClose = React.useCallback(() => {
appStateApi.setView({ kind: 'page', nodeId: page.id });
}, [appStateApi, page.id]);
if (!dialogState?.isDraft) {
Copy link
Member

Choose a reason for hiding this comment

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

Prefer positive logic

Suggested change
if (!dialogState?.isDraft) {
if (dialogState?.isDraft) {
setDialogState(null);
// ...

if (currentView.kind === 'page' && currentView.view?.kind === 'query') {
const node = appDom.getNode(dom, currentView.view?.nodeId, 'query');
return { node, isDraft: false };
}

return null;
if (!dialogState?.isDraft) {
Copy link
Member

Choose a reason for hiding this comment

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

Same,

Suggested change
if (!dialogState?.isDraft) {
if (dialogState?.isDraft) {
return previousState;
}
return null;

});
}, [dom, currentView]);
}, [dom, currentView, dialogState?.isDraft]);
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps extract this in a variable and reuse?

const isDraft = !!dialogState?.isDraft;

Copy link
Member

@Janpot Janpot left a comment

Choose a reason for hiding this comment

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

Few stylistic nits. Should we add/amend a test for this?

@apedroferreira
Copy link
Member Author

Should we add/amend a test for this?

Not sure, this was quite a specific issue since it involves switching tabs. The dom object changes when we switch tabs and it's a dependency in the effect that closes the query editor, so it was running unintentionally.
I can add a test, I don't think it's the most obvious case to write a test for, but if we don't mind the extra test it also offers extra coverage. I might just try to include switching tabs in an existing test for the query editor not to add a whole new test too, I'll give it a try.

@apedroferreira
Copy link
Member Author

Few stylistic nits. Should we add/amend a test for this?

I've modified the rest-basics test since it already tested the query editor a bit.

@apedroferreira apedroferreira merged commit b43fbd4 into master Mar 24, 2023
@apedroferreira apedroferreira deleted the fix-query-editor-change-tab branch March 24, 2023 14:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Query editor in draft state closes after switching tabs
2 participants