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

Moved theme Map and added API to use it from other dependencies #12861

Merged
merged 13 commits into from
Aug 5, 2022
Merged

Moved theme Map and added API to use it from other dependencies #12861

merged 13 commits into from
Aug 5, 2022

Conversation

JohanMabille
Copy link
Member

@JohanMabille JohanMabille commented Jul 26, 2022

This fixes some issues reported in #12812:

  • matchBtackets
  • smartIndent
  • codeFolding
  • foldGutter

This also adds a token for modes and themes registry.
This PR is not finished but a review would be appreciated to know if it goes in the right direction. If so I would add missing doc strings and redirection.

I also wonder if we should expose more methods from Mode via the IEditorModeRegistry interface.

@jupyterlab-probot
Copy link

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

Copy link
Member

@fcollonval fcollonval left a comment

Choose a reason for hiding this comment

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

Using tokens is nice. But this requires changing a bit the current structure to not use static object. In particular the editorThemeRegistry. They should be propagated to the EditorConfiguration. Or the logic should be similar to the EditorModeRegistry that modifies a static list.

Otherwise, providing a different theme registry overriding the token will not have the expected outcome.

If we want to align more with other APIs, the generic pattern are:

  • tokens that define registries
  • registries provide factories
  • factories instantiate objects

But that would require more work for the code editor.

@JohanMabille
Copy link
Member Author

JohanMabille commented Jul 28, 2022

@fcollonval I removed the token API for now, so we don't have backward incompatible changes when we add it later, and I added an API for getting and adding themes similar to that of Mode.

Or the logic should be similar to the EditorModeRegistry that modifies a static list.

Not sure to understand why this would work. The EditorConfiguration works with functions that use the static list, so even if another extension defines another EditorModeRegistry, this later won't be used by the EditorConfiguration object and we'll run into the same issue as with the previous definition ot editorThemeRegistry.

My understanding is that we would also need to propagate the mode registry object to the editor configuration object to get the expected behavior. But that can be done after 4.0.

@JohanMabille JohanMabille changed the title Added token for modes and themes registries Moved theme Map and added API to use it from other dependencies Jul 28, 2022
@fcollonval
Copy link
Member

I'm taking over this PR.

I added a test for adding a mode (not yet pushed)
I was testing adding a theme but actually the reconfiguration of the editors are not happening. So I'm working to fix that.

@jtpio jtpio mentioned this pull request Aug 3, 2022
34 tasks
@github-actions github-actions bot added Design System CSS pkg:codeeditor tag:CSS For general CSS related issues and pecadilloes labels Aug 4, 2022
@fcollonval
Copy link
Member

bot please update snapshots

Use codemirror theme example to test theme registration
@fcollonval
Copy link
Member

bot please update snapshots

@fcollonval
Copy link
Member

bot please update snapshots

@fcollonval
Copy link
Member

bot please update galata snapshots

@fcollonval
Copy link
Member

Kicking the CI

@fcollonval fcollonval merged commit 9adb49c into jupyterlab:master Aug 5, 2022
@JohanMabille JohanMabille deleted the config branch August 22, 2022 08:39
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 23, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants