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

link about tab to menu #1060

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from
Draft

link about tab to menu #1060

wants to merge 6 commits into from

Conversation

Sebastian-ubs
Copy link
Contributor

@Sebastian-ubs Sebastian-ubs commented Aug 8, 2024

This change is Reviewable

fixes #977

grafik

@Sebastian-ubs
Copy link
Contributor Author

Sebastian-ubs commented Aug 9, 2024

@Sebastian-ubs Sebastian-ubs changed the title try to link about tab to menu (not working) link about tab to menu Aug 9, 2024
@Sebastian-ubs
Copy link
Contributor Author

Sebastian-ubs commented Aug 9, 2024

Now this works.

TODOs:

  • rename "about" to "showAboutDialog" to adher to the code style guide
  • check if layout works without a last single tab (in here switched to "test tab 1", but if possible remove this)
  • get a PR review

So either keep this pr open until I am back in September or work on those remaining things on your own :-)

@Sebastian-ubs
Copy link
Contributor Author

Sebastian-ubs commented Sep 11, 2024

Also without tabs Platform can be started and new tabs can be added via "Open Scripture Editor" or "Open Resource Viewer"

grafik

Completely idependent of this change: Throwing away the last tab produces an error, but everything seems to continue to work

react-dom.development.js:86 Warning: Attempted to synchronously unmount a root while React was already rendering. React cannot finish unmounting the root until the current render has completed, which may lead to a race condition.
at div
at DockBox (http://localhost:1212/renderer.dev.js:54158:9)
at div
at DockLayout (http://localhost:1212/renderer.dev.js:54706:9)
at DockLayoutWrapper (http://localhost:1212/renderer.dev.js:113249:106)
at PlatformDockLayout (http://localhost:1212/renderer.dev.js:113999:72)
at Main
at RenderedRoute (http://localhost:1212/renderer.dev.js:102295:5)
at Routes (http://localhost:1212/renderer.dev.js:102985:5)
at Router (http://localhost:1212/renderer.dev.js:102919:15)
at MemoryRouter (http://localhost:1212/renderer.dev.js:102814:5)
at App

@Sebastian-ubs Sebastian-ubs marked this pull request as ready for review September 11, 2024 08:16
@Sebastian-ubs
Copy link
Contributor Author

Sebastian-ubs commented Sep 11, 2024

Note that new dialogs open up each time, that you click the menu entry, but I think this is fine for now - maybe even good for playing with many windows.
Also min size for dialogs and tabs is something to consider in a future user story independent of this.

@Sebastian-ubs Sebastian-ubs enabled auto-merge (squash) September 11, 2024 08:28
Copy link
Member

@tjcouch-sil tjcouch-sil left a comment

Choose a reason for hiding this comment

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

Thanks for the hard work on this! Looks like a good improvement! Few things to adjust.

Sadly, for some reason, when I delete dock-saved-layout from localstorage, refresh, open the about tab, then open the scripture editor, the about tab disappears and no scripture editor appears. Things get even weirder after that. I haven't seen this problem before, but I also pretty much haven't ever closed all my tabs since I discovered long ago the problem with not being able to dock things once you close all tabs. Because this seems to make more apparent the unstable situation with no tabs, I think we will need to wait to merge this til after we make the demo branch.

Reviewed 2 of 10 files at r2, 9 of 10 files at r4, 1 of 1 files at r5, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @Sebastian-ubs)


extensions/src/platform-scripture/src/main.ts line 146 at r5 (raw file):

  );

  const showAboutDialogPromise = papi.commands.registerCommand('platform.about', async () =>

This command belongs in core, not in an extension. Could you maybe move this to the dialog service? Maybe similar to how these web view service commands are registered https://github.com/paranext/paranext-core/blob/link-about-tab-to-menu/src/renderer/services/web-view.service-host.ts#L1344


extensions/src/platform-scripture/src/main.ts line 147 at r5 (raw file):

  const showAboutDialogPromise = papi.commands.registerCommand('platform.about', async () =>
    papi.dialogs.showAboutDialog(),

I believe this needs to be returned in order to make this command function properly wait on the dialog.


extensions/src/platform-scripture/src/types/platform-scripture.d.ts line 653 at r5 (raw file):

  export interface CommandHandlers {
    'platform.about': (projectId?: string | undefined) => Promise<string | undefined>;

This would also move to papi-shared-types.ts when moving the command. Also please add a JSDoc :)


src/shared/services/dialog.service-model.ts line 37 at r5 (raw file):

   *
   * @param options Various options for configuring the dialog that shows
   * @returns Returns the user's selected project id or `undefined` if the user cancels

Is anything returned from the about dialog? Should this be changed?

Copy link
Member

@tjcouch-sil tjcouch-sil left a comment

Choose a reason for hiding this comment

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

Maybe the disappearing is somehow related to it being a dialog. We should probably adjust things so opening a dialog doesn't change the latest place a tab was opened. That way, new tabs won't open as a tab in the same group as dialogs.

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @Sebastian-ubs)

@irahopkinson
Copy link
Contributor

@Sebastian-ubs nudge on this PR. If you aren't finishing this soon then perhaps close it?

@Sebastian-ubs Sebastian-ubs marked this pull request as draft November 15, 2024 09:58
auto-merge was automatically disabled November 15, 2024 09:58

Pull request was converted to draft

@Sebastian-ubs
Copy link
Contributor Author

Thanks, I wanted to work on it, but has no prio right now. Changes done in here are still valid, but seem not enough. Will wait until https://github.com/paranext/ux/issues/94 gets priority.

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.

Link the about tab to the main menu's "about" entry
3 participants