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

Implement Workspaces GUI #15946

Merged
merged 30 commits into from
Mar 25, 2024
Merged

Conversation

krassowski
Copy link
Member

@krassowski krassowski commented Mar 7, 2024

References

Closes #6944

Code changes

  • adds dedicated workspaces and workspaces-extension packages
  • moves existing workspace logic for "save", "save as" and "open" (and commands) from apputils-extension:workspaces (which retains only the mime extension for opening .jupyter-workspace files on click)
  • adds workspaces-extension:sidebar plugin which populates the running sidebar with the workspaces as per mockups in Create a workspaces GUI #6944
  • adds workspaces-extension:menu plugin which populates a "Workspaces..." submenu in the main "File" menu; the existing "Save Workspace" and "Save Workspace As..." menu entries are moved to this new sub-menu.
  • adds workspaces-extension:commands plugin which implements all the new commands and adds them to the command palette
  • improves dialog and file dialog APIs to enable implementation of dialogs from mockups in Create a workspaces GUI #6944
Other (less relevant) code changes
  • simpliies saveAs logic by using InputDialog.getText() (which gives us pre-selection and placeholder)
  • removes duplicated licence banner in filebrowser/style/base.css
  • allows to set defaultButton on input dialogs; this is just passed to dialog which supports the defaultButton already

User-facing changes

workspaces

Backwards-incompatible changes

None

Fix sidebars and commands missing await

Add the menus and sidebars schema

Fixup integrity

Fix yarn lock

Fix margin around close buttons in sidebar
@krassowski krassowski added this to the 4.2.0 milestone Mar 7, 2024
Copy link

Thanks for making a pull request to jupyterlab!
To try out this branch on binder, follow this link: Binder

workspace from file, adding workspaces by command, and context menu.
Fix minor issue with docs formatting/linking.
@JasonWeill
Copy link
Contributor

@krassowski Thanks for your work so far on this! Please let me know when you'd like some feedback on it.

@krassowski krassowski marked this pull request as ready for review March 11, 2024 12:31
@krassowski
Copy link
Member Author

Note to reviewers: failures of npm dependencies failed to install are expected because this PR adds a new package which is not on npm yet. These will get resolved once we merge and follow with a new pre-release.

packages/apputils/src/inputdialog.ts Outdated Show resolved Hide resolved
packages/filebrowser/style/base.css Show resolved Hide resolved
docs/source/user/workspaces.rst Show resolved Hide resolved
docs/source/user/workspaces.rst Show resolved Hide resolved
docs/source/user/workspaces.rst Show resolved Hide resolved
docs/source/user/workspaces.rst Show resolved Hide resolved

The `metadata` must be a mapping with an `id`
key that has the same value as the ID of the workspace. This should also be the relative URL path to access the workspace,
like `/lab/workspaces/foo`. Additionally, `metadata` may contain `created` and `last_modified` fields with date and time creation and most recent modification, respectively. The date and time are encoded using ISO 8601 format.
Copy link
Member Author

Choose a reason for hiding this comment

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

Done in 8134d1f.

@@ -7,6 +7,7 @@ import type { ISettingRegistry } from '@jupyterlab/settingregistry';
const menuPaths = [
'File',
'File>New',
'File>Workspaces…',
Copy link
Member Author

Choose a reason for hiding this comment

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

Done in 2872d00.

docs/source/user/workspaces.rst Outdated Show resolved Hide resolved
Co-authored-by: Jason Weill <93281816+JasonWeill@users.noreply.github.com>
@krassowski
Copy link
Member Author

Thank you for the review @JasonWeill!

The menu should use ellipses consistently. Apple's Human Interface Guidelines for menus suggest, "Append an ellipsis to a menu item’s label when people need to provide additional information before the action can complete." A cascading menu, like File > Workspaces, should not have an ellipsis (as it does now), but all of its submenu items require additional information in a modal dialog, so they should all have ellipses.

Done in 2872d00. What I was trying to do was being consistent with "Save and Export Notebook As..." just above which does have an ellipsis. Do you think that should be changed too (if so, let's open an issue to track it)?

In the running pane’s “Workspaces” section“, when I hover over the trash icon to the right of each workspace, the tooltip only says ”Delete“. For accessibility and usability, it should say ”Delete %1“, with the workspace name substituted in. (”Delete workspace ‘%1’“ could also work.)

Done in 19f3ad8.

The confirmation for deleting a workspace doesn’t put the name of the workspace in quotes or otherwise emphasize it, so users may miss it. This will also avoid confusion in case the workspace’s name is a word.

Done in f15ed80.

The first sentence of the confirmation dialog for deleting a workspace reads, “Deleting workspace %1 will also delete its URL.” I’m not sure what this means. The URL itself is not deleted, but visiting it will no longer take the user to workspace %1. Consider alternate wording, such as, “If you delete workspace ‘%1’, URLs that refer to this workspace will go to a different workspace.”

This wording was taken from mockups in #6944. While I agree that "deleting a URL" sounds odd, I do not think that "URLs that refer to this workspace will go to a different workspace" is accurate (it will not - it will either show an error dialog that workspace does not exist and load the default workspace, or create a new one; I have no idea why it sometimes does one or the other). I suggest to discuss this in #6944 and we can adjust it after this PR is merged (as it staying open has a high cost due to merge conflicts).

After I click a link to go to the auto-w workspace, when I open the running widget in the left pane, then I expand workspaces, language servers, kernels, and open tabs, in that order, I see “language servers” is very tall and empty, and both “kernels” and “workspaces” have content in them but are very small:

You can resize them by dragging. Changes to the logic for managing sizes of the panels is out of scope for this PR, but if you want to suggest a better behaviour I would advise to open a new issue.

The “Delete All” dialog needs work. Its title is in Title Case and ends in a question mark (Delete All?) whereas the dialog title for deleting a single workspace is in sentence case and has no punctuation (Delete workspace).

This comes from the running package code:

const shutdownAllLabel =
options.manager.shutdownAllLabel || trans.__('Shut Down All');
const shutdownTitle = `${shutdownAllLabel}?`;
const shutdownAllConfirmationText =
options.manager.shutdownAllConfirmationText ||
`${shutdownAllLabel} ${options.manager.name}`;
this.addClass(SECTION_CLASS);
this.title.label = options.manager.name;
function onShutdown() {
void showDialog({
title: shutdownTitle,
body: shutdownAllConfirmationText,
buttons: [
Dialog.cancelButton(),
Dialog.warnButton({ label: shutdownAllLabel })
]
}).then(result => {
if (result.button.accept) {
options.manager.shutdownAll();
}
});

Other dialogs, like those for shutting down all kernels or opening from an arbitrary path, use Title Case, and some end in a question mark.

Yes, because they all generated by running package code. I would suggest that changes to that are handled in separate follow up PR as these might lead to broader API changes, best discussed separately.

In addition, the “Delete All” dialog does not convey that deleting all workspaces is irreversible (cannot be undone), whereas the "Delete workspace" dialog does. I think that it’s important to convey that something cannot be undone as prominently as possible.

Done in c257f6d.

Copy link
Contributor

@JasonWeill JasonWeill 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 opening related issues! I think we're good to proceed now.

@krassowski
Copy link
Member Author

bot please update snapshots

Copy link
Contributor

Documentation snapshots updated.

Copy link
Contributor

Galata snapshots updated.

@krassowski
Copy link
Member Author

krassowski commented Mar 21, 2024

Note to self: one docs test needs work before merging:

    [documentation] › test/documentation/workspaces.test.ts:35:7 › Workspaces sidebar › Workspaces context menu 

@krassowski krassowski marked this pull request as draft March 21, 2024 20:02
@krassowski krassowski marked this pull request as ready for review March 22, 2024 09:28
@krassowski
Copy link
Member Author

I'm planning to merge and cut a new pre-release early on Monday.

@krassowski krassowski merged commit 2fc6682 into jupyterlab:main Mar 25, 2024
73 of 81 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Create a workspaces GUI
2 participants