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

PR: Localize strings in JLab #8800

Merged
merged 1 commit into from Aug 11, 2020
Merged

Conversation

goanpeca
Copy link
Member

@goanpeca goanpeca commented Aug 8, 2020

References

Code changes

User-facing changes

Backwards-incompatible changes

To be completed:

  1. File/Edit/Kernel/Run menu extensions that used constants for action/noun/plural/name have now been replaced to functions that take the amount of items so that the pluralization can be correctly handled for different languages.

  2. IEditorServices provides now 2 functions getFactoryService, getMimeTypeService(that accept an optional translator instead of factoryService and typeService.

/**
 * Code editor services.
 */
export interface IEditorServices {
  /**
   * The code editor factory.
   */
  getFactoryService(translator?: ITranslator): IEditorFactoryService;

  /**
   * The editor mime type service.
   */
  getMimeTypeService(translator?: ITranslator): IEditorMimeTypeService;
}

@goanpeca goanpeca changed the title Fixing trans WIP: Fixing trans Aug 8, 2020
@jupyterlab-dev-mode
Copy link

Thanks for making a pull request to JupyterLab!

To try out this branch on binder, follow this link: Binder

@github-actions github-actions bot added tag:Design System CSS If a PR is editing any CSS files please add this tag for design team to review. pkg:application pkg:apputils pkg:codemirror pkg:console tag:CSS For general CSS related issues and pecadilloes documentation labels Aug 8, 2020
@goanpeca goanpeca force-pushed the fixing-trans branch 3 times, most recently from c236919 to 5c9d354 Compare August 10, 2020 02:44
@goanpeca goanpeca changed the title WIP: Fixing trans PR: Localize strings in JLab Aug 10, 2020
out.txt Outdated Show resolved Hide resolved
@blink1073
Copy link
Member

I'll be afk for the next 3 hours or so, but will be focusing on getting this done after that.

@goanpeca
Copy link
Member Author

goanpeca commented Aug 10, 2020

I'll be afk for the next 3 hours or so, but will be focusing on getting this done after that.

As far as I can tell there I sno need to change any API, except for the noun and plural stuff in the menus (and even we could make the changes optional and backwards compatible anyway). So I could open a PR just with that and make the rest of the changes in separate PRs and not in a hurry.

Still let's talk to go over the details. Thanks!

@jasongrout jasongrout mentioned this pull request Aug 10, 2020
43 tasks
@goanpeca goanpeca marked this pull request as ready for review August 10, 2020 18:14
Untitled.ipynb Outdated Show resolved Hide resolved
Copy link
Member

@blink1073 blink1073 left a comment

Choose a reason for hiding this comment

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

Reviewed the first 110 files

packages/apputils/src/mainareawidget.ts Outdated Show resolved Hide resolved
packages/apputils/src/sessioncontext.tsx Outdated Show resolved Hide resolved
packages/apputils/src/sessioncontext.tsx Outdated Show resolved Hide resolved
packages/apputils/src/sessioncontext.tsx Outdated Show resolved Hide resolved
packages/celltags/src/addwidget.ts Outdated Show resolved Hide resolved
packages/celltags/src/tool.ts Outdated Show resolved Hide resolved
packages/codeeditor/src/tokens.ts Outdated Show resolved Hide resolved
packages/console-extension/src/index.ts Outdated Show resolved Hide resolved
Copy link
Member

@blink1073 blink1073 left a comment

Choose a reason for hiding this comment

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

I didn't look closely at the FIXME comments in this pass

packages/fileeditor-extension/src/commands.ts Outdated Show resolved Hide resolved
packages/translation/src/gettext.ts Outdated Show resolved Hide resolved
@goanpeca
Copy link
Member Author

I didn't look closely at the FIXME comments in this pass

Looking into them after I make the next push (moving the factories changed a lot, so that is also a breaking change I guess... since It is also being used in the exampleS)

@goanpeca goanpeca force-pushed the fixing-trans branch 2 times, most recently from ceb36b8 to 3ac627c Compare August 11, 2020 14:31
@goanpeca goanpeca force-pushed the fixing-trans branch 3 times, most recently from 1022065 to 3c98117 Compare August 11, 2020 19:01
@goanpeca goanpeca closed this Aug 11, 2020
@goanpeca goanpeca reopened this Aug 11, 2020
@blink1073 blink1073 closed this Aug 11, 2020
@blink1073 blink1073 reopened this Aug 11, 2020
@goanpeca
Copy link
Member Author

goanpeca commented Aug 11, 2020

I also changed the FIXME: comments to FIXME-TRANS: so we can tackle them faster.

@blink1073
Copy link
Member

To fix examples:

  • add @jupyterlab/translation-extension to examples/app/index.js
  • add @jupyterlab/translation-extension and @jupyterlab/translation deps in examples/app/package.json
  • add @jupyterlab/translation-extension and @jupyterlab/translation deps in examples/federated/core_package/package.json
  • also put both them in resolutions and put the extension in extensions in that same core_package

@afshin
Copy link
Member

afshin commented Aug 11, 2020

I think this is causing the file browser plugin to fail:

diff --git a/packages/filebrowser-extension/src/index.ts b/packages/filebrowser-extension/src/index.ts
index 90f662256..190e35c96 100644
--- a/packages/filebrowser-extension/src/index.ts
+++ b/packages/filebrowser-extension/src/index.ts
@@ -158,7 +158,7 @@ const factory: JupyterFrontEndPlugin<IFileBrowserFactory> = {
   activate: activateFactory,
   id: '@jupyterlab/filebrowser-extension:factory',
   provides: IFileBrowserFactory,
-  requires: [ILabShell, IDocumentManager, IDocumentManager, ITranslator],
+  requires: [ILabShell, IDocumentManager, ITranslator],
   optional: [IStateDB, IRouter, JupyterFrontEnd.ITreeResolver]
 };

@goanpeca
Copy link
Member Author

Thanks for the help @afshin ! I will add a check for number of parameters on requires and also check if a same plugin has been registered before. That very stupid error on my side was not providing the right feedback :-p

Copy link
Member

@blink1073 blink1073 left a comment

Choose a reason for hiding this comment

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

Let's merge and iterate

@blink1073 blink1073 merged commit 4e7c446 into jupyterlab:master Aug 11, 2020
@goanpeca goanpeca deleted the fixing-trans branch August 11, 2020 23:24
@blink1073 blink1073 added this to the 3.0 milestone Sep 11, 2020
@github-actions github-actions bot added the status:resolved-locked Closed issues are locked after 30 days inactivity. Please open a new issue for related discussion. label Mar 11, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 11, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-change A change that should be accompanied by a major version increase documentation pkg:application pkg:apputils pkg:celltags pkg:codemirror pkg:console status:resolved-locked Closed issues are locked after 30 days inactivity. Please open a new issue for related discussion. tag:CSS For general CSS related issues and pecadilloes tag:Design System CSS If a PR is editing any CSS files please add this tag for design team to review. tag:Examples tag:i18n
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants