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
Harmonize the use of ellipses in command and menu items #16237
Conversation
Adjusted wording to satisfy issue jupyterlab#16321 Use "Folder", not "Directory", in file browser settings jupyterlab#16231
File>Save and Export Notebook As -Added elipses to: File>Close and Shut Down Notebook Help>Show Keyboard Shortcuts There remain some dual nature menu items that may or may not pop up a prompt depending on save history of current open files. I did not change these elipse situations Examples: File > Save and File > Close All Tabs IAW standards mentioned in issue jupyterlab#16025
File>Save and Export Notebook As -Added elipses to: File>Close and Shut Down Notebook Help>Show Keyboard Shortcuts There remain some dual nature menu items that may or may not pop up a prompt depending on save history of current open files. I did not change these elipse situations Examples: File > Save and File > Close All Tabs IAW standards mentioned in issue jupyterlab#16025
Thanks for making a pull request to jupyterlab! |
@meeseeksdev tag bug |
Several of the test cases now fail, because the menus are different. You can comment with |
Documentation snapshots updated. |
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.
"Save Current Workspace…" should not have an ellipsis, because if the workspace already has a name, this command does not require user attention.
If the current workspace has not yet been saved, the command will call "Save Current Workspace As…", which has an ellipsis, and should keep it.
.gitignore
Outdated
@@ -148,3 +148,4 @@ packages/lsp/src/_* | |||
# generated reports | |||
webpack-bundle-analyzer.html | |||
examples/**/test-results/ | |||
docker/start.sh |
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 file isn't relevant to this change. Can you please remove this line from .gitignore
, and remove the empty docker/start.sh
file from your branch?
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.
I will attempt to do so, it was not intended to be in the commit.
@@ -650,7 +650,7 @@ const utilityCommands: JupyterFrontEndPlugin<void> = { | |||
}); | |||
|
|||
commands.addCommand(CommandIDs.displayShortcuts, { | |||
label: trans.__('Show Keyboard Shortcuts'), | |||
label: trans.__('Show Keyboard Shortcuts...'), |
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.
Please use the ellipsis character …, which looks very similar to three periods, but which is not the same.
- Ellipsis:
…
- Three periods:
...
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.
Thanks, i will redo the code changes to fix this.
Galata snapshots updated. |
for more information, see https://pre-commit.ci
Thank you for your patience Jason, trying not to make life harder on others while I learn. I think I've satisfied your feedback. bot please update snapshots |
Documentation snapshots updated. |
Galata snapshots updated. |
The failure in:
appears relevant, the test will need updating. |
@@ -2417,7 +2417,7 @@ function addCommands( | |||
isEnabled | |||
}); | |||
commands.addCommand(CommandIDs.closeAndShutdown, { | |||
label: trans.__('Close and Shut Down Notebook'), | |||
label: trans.__('Close and Shut Down Notebook…'), |
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.
Why is an ellipsis needed here?
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.
Ah because it shows a confirmation dialog.
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.
Thank you!
Audited all menu-bar options for required ellipses or removal of ellipses.
As mentioned in the commit, a few items required ellipses at certain times while not requiring them at other times. Those dual nature elements were not adjusted.
References
#16025
Code changes
/packages/notebook-extension/schema/export.json line 16 removed ellipses from label
/packages/notebook-extension/index.ts line 2420 added ellipses to label
/packages/apputils-extension/index.ts line 653 added ellipses to label
User-facing changes
Adjusted ellipses to conform with the guidance posted in the issue.
Added ellipses to Show Keyboard Shortcuts under help menu.
Added ellipses to Close and Shut Down Notebook under file menu.
Removed ellipses on Save and Export Notebook As under file menu.
Updated areas:
Backwards-incompatible changes
None