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

web: don't remove properties pane when editor is refocused #7780

Conversation

01zulfi
Copy link
Collaborator

@01zulfi 01zulfi commented Mar 18, 2025

Signed-off-by: 01zulfi [email protected]

Closes #7770

@01zulfi 01zulfi force-pushed the web-beta/toggle-properties-on-request-focus branch from 24b3a7b to f664c61 Compare March 18, 2025 11:11
@01zulfi 01zulfi changed the title web: don't remove properties pane when editor is refocused web: don't remove toc pane when editor is refocused Mar 18, 2025
@01zulfi 01zulfi requested a review from ammarahm-ed March 18, 2025 11:11
Comment on lines 1174 to 1176
if (toggleTableOfContents !== false) {
this.toggleTableOfContents(false);
}
Copy link
Collaborator

@ammarahm-ed ammarahm-ed Mar 18, 2025

Choose a reason for hiding this comment

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

In what case we want to toggle table of contents with properties? I think this can be removed as it's not needed anymore. There is a dedicated function to toggle TOC already which should be used. The fact that this did no affect any other code shows it isn't used anywhere in the repo.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Whenever properties is toggled, it closes the table of contents, this ensures that only one pane is open at a time

Copy link
Collaborator

@ammarahm-ed ammarahm-ed Mar 18, 2025

Choose a reason for hiding this comment

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

Let's keep the existing behavior from the stable app and:

  1. If you open TOC, it should close properties if already opened
  2. If you close properties, do nothing with the TOC.
  3. If you open properties, do nothing with the TOC.

From what I can tell, it is a bug in the beta app because the stable version behaves correctly (it doesn't close properties on opening TOC though which we should fix here). The TOC never closes unless you close it by clicking X.

Copy link
Collaborator

@ammarahm-ed ammarahm-ed Mar 18, 2025

Choose a reason for hiding this comment

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

Just trying the beta app again, it seems that the UI has changed quite a bit and now properties and TOC are part of the editor, not floating on top. Both show in the same container, are they stacked on top or replaced?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

They are separate panes

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@ammarahm-ed do let me know what needs to be done

Comment on lines 385 to 387
onRequestFocus: () => {
toggleProperties(false, false);
},
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of all these changes, let's just remove this "close on focus" logic altogether.

@01zulfi 01zulfi force-pushed the web-beta/toggle-properties-on-request-focus branch from f664c61 to cfcb08a Compare March 25, 2025 10:37
@01zulfi 01zulfi force-pushed the web-beta/toggle-properties-on-request-focus branch from cfcb08a to 7fa4860 Compare March 25, 2025 10:39
@01zulfi 01zulfi requested a review from thecodrr March 25, 2025 10:40
@01zulfi 01zulfi changed the title web: don't remove toc pane when editor is refocused web: don't remove properties pane when editor is refocused Mar 25, 2025
@thecodrr thecodrr merged commit 8f92335 into streetwriters:beta Mar 25, 2025
4 of 14 checks passed
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.

3 participants