Conversation
brichet
left a comment
There was a problem hiding this comment.
Thanks a lot @jtpio for working on this, it looks great!
Some missing translation that we may want to handle (in chat model only):
jupyter-chat/packages/jupyterlab-chat/src/model.ts
Lines 420 to 421 in c13de66
jupyter-chat/packages/jupyterlab-chat/src/model.ts
Lines 442 to 443 in c13de66
jupyter-chat/packages/jupyterlab-chat/src/model.ts
Lines 486 to 487 in c13de66
brichet
left a comment
There was a problem hiding this comment.
Thanks @jtpio.
We'll probably need to update jupyterlab/language-packs#903 with the translation domain.
andrii-i
left a comment
There was a problem hiding this comment.
Overall looks good. Please see one concern below worth noting and potentially addressing.
| export interface IOptions<T extends LabChatPanel> | ||
| extends DocumentRegistry.IWidgetFactoryOptions<T>, | ||
| Omit<Chat.IOptions, 'model' | 'inputToolbarRegistry'> { | ||
| Omit<Chat.IOptions, 'model' | 'inputToolbarRegistry' | 'translator'> { |
There was a problem hiding this comment.
translator is omitted here so chat widgets opened via the file manager won't be translated - useTranslator() will fall back to nullTranslator for the entire component tree while side-panel mode correctly gets localization via MultiChatPanel. Worth extending ChatWidgetFactory.IOptions to accept and forward translator so document mode has full coverage too. Can be addressed in a follow-up PR.
There was a problem hiding this comment.
This interface already extends DocumentRegistry.IWidgetFactoryOptions which includes a translator.
The property would be duplicated if not omitted, I think.
Co-Authored-By: Nicolas Brichet <brichet@users.noreply.github.com>
|
Ah just realized this picked up a conflict. Just rebased to fix it. |
|
Thanks @jtpio |
Fixes #49
Companion PR: jupyterlab/language-packs#903