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

fix: disable special colors when in vscode dark mode #152

Merged
merged 1 commit into from
Oct 9, 2023

Conversation

jokasimr
Copy link
Collaborator

@jokasimr jokasimr commented Sep 29, 2023

Part of #87

Currently all this does is

  1. it replaces the use of --jp-content-font-color variable with a custom --sca-font-color variable
  2. it disables custom font colors when we are in VScode.

To fix this we should figure out:

  1. What colors should be used on what fields in what environments? (jupyter, pycharm, vscode)
  2. How do we determine if the notebook is running in pycharm?

@jl-wynen jl-wynen added the sprint-scipp-2023-09 Sprint of the Scipp team label Sep 29, 2023
@@ -0,0 +1,13 @@
:root {
--sca-font-color0: var(--jp-content-font-color0);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you change the prefix to cean? I used that for the HTML classes.

@jokasimr jokasimr marked this pull request as ready for review October 2, 2023 07:12
@lru_cache(maxsize=1)
def dataset_style() -> str:
sheet = _preprocess_style(_read_text("dataset.css", "styles"))
return f"<style>{sheet}</style>"
return f"{common_style()}<style>{sheet}</style>"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you just merge them into a single <style> tag?

--cean-font-color3: var(--jp-content-font-color3);
}

body.vscode-dark {
Copy link
Collaborator

Choose a reason for hiding this comment

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

What about light mode?

@jl-wynen
Copy link
Collaborator

jl-wynen commented Oct 3, 2023

Can you add a release note?

@jokasimr
Copy link
Collaborator Author

jokasimr commented Oct 6, 2023

This change doesn't do anything about the light background in the sphinx output.
It'd unfortunately be hard to fix in a good way because the pydata sphinx theme explicitly uses the same styling for notebook output in light/dark themes: see comment here.

Haven't implemented any special case for pycharm either.

@jokasimr
Copy link
Collaborator Author

jokasimr commented Oct 6, 2023

I've checked the different platforms (Jupyter, Vscode, Pycharm, Sphinx docs) and they are readable in dark and light mode. Pycharm and sphinx don't use custom font colors (e.g. for missing values etc).

I think this can be merged now.

Copy link
Collaborator

@jl-wynen jl-wynen left a comment

Choose a reason for hiding this comment

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

Almost there 👍

Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you place comments that explain the different selectors? In particular the distinction between data-theme and data-jp-theme-light.

Copy link
Collaborator Author

@jokasimr jokasimr Oct 6, 2023

Choose a reason for hiding this comment

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

Good question 🤔

data-theme is commonly used to toggle between dark and light mode, in particular it is used to toggle dark/light mode in the Sphinx docs.
However, since the pydata sphinx theme disables dark mode for notebook output it is not used. So perhaps better to remove it?

data-jp-theme-light is used by Jupyter to toggle between dark and light mode.

@jokasimr jokasimr force-pushed the colors-in-html-repl branch 2 times, most recently from 0e42c26 to b0b8145 Compare October 6, 2023 13:09
@jl-wynen jl-wynen merged commit effa5ca into main Oct 9, 2023
13 checks passed
@jl-wynen jl-wynen deleted the colors-in-html-repl branch October 9, 2023 10:07
@jl-wynen jl-wynen mentioned this pull request Oct 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
sprint-scipp-2023-09 Sprint of the Scipp team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants