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

Use a theme CSS variable instead of a Material Design color for the highlight animation #124

Open
joaopalmeiro opened this issue Jul 11, 2024 · 5 comments · May be fixed by #126
Open

Use a theme CSS variable instead of a Material Design color for the highlight animation #124

joaopalmeiro opened this issue Jul 11, 2024 · 5 comments · May be fixed by #126

Comments

@joaopalmeiro
Copy link

joaopalmeiro commented Jul 11, 2024

Hi! 👋

Context

I am creating a JupyterLab theme and as jupyterlab-execute-time is one of the extensions used, I would like to be able to guarantee the customization of its styles completely using the CSS variables defined in JupyterLab themes.

Problem

Currently, one of the colors used in the highlight animation is one of the Material Design colors and not one of the semantic colors defined in JupyterLab themes, for example:

/* Transition to highlight a cell change */
@keyframes executeHighlight {
from {
background-color: var(--md-blue-100, #9fccff);
}
to {
background-color: var(--jp-cell-editor-background);
}
}

Possible solution

The Material Design color used, --md-blue-100, corresponds to --jp-brand-color3 in the default JupyterLab themes:

So, a possible solution would be to change var(--md-blue-100, #9fccff) to var(--jp-brand-color3, #bbdefb), using the current Material Design color as a fallback.

Let me know your thoughts, please, and if I can open a PR. Thanks! 😄

@krassowski
Copy link
Collaborator

I am not sure if it should be --jp-brand-color3 though, as this can result in poor color contrast if the brand color is modified; one alternative to consider would be one of the --jp-accent-colors but these are green not blue.

I think that using a variable is a good idea. I think the execution highlight should be a variable itself, something like:

--jp-execute-time-highlight: var(--jp-brand-color3, #bbdefb);
/* ... */
background-color: var(--jp-execute-time-highlight); 

This way if a theme wants to modify --jp-brand-color3 it can also easily swap --jp-execute-time-highlight to avoid contrast issues.

@joaopalmeiro
Copy link
Author

joaopalmeiro commented Jul 15, 2024

This seems like a perfect solution to me, @krassowski!

Based on this, I would also like to suggest a variable (e.g., --jp-execute-time-background: var(--jp-cell-editor-background)) for the background color of this cell extension for consistency.

In my case, I override the style so that the background color is that of the notebook (white) and not the cell (gray), so that it becomes something more "secondary" in the interface.

Thoughts?

@krassowski
Copy link
Collaborator

Sounds good to me.

@joaopalmeiro
Copy link
Author

joaopalmeiro commented Jul 15, 2024

@krassowski, can I go ahead and open a PR to propose this change, or should we wait for more feedback?

@krassowski
Copy link
Collaborator

Personally, I think this change would be uncontroversial and in your place I would go ahead with a PR (having in mind that during the review other solutions may be suggested).

@joaopalmeiro joaopalmeiro linked a pull request Jul 15, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants