deshaw / jupyterlab-execute-time

A JupyterLab extension for displaying cell timings
BSD 3-Clause "New" or "Revised" License
361 stars 48 forks source link

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

Open joaopalmeiro opened 1 month ago

joaopalmeiro commented 1 month ago

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: https://github.com/deshaw/jupyterlab-execute-time/blob/985728d1b9889f52a8c0f3a646739849c0f9bbac/style/base.css#L1-L9

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

Sounds good to me.

joaopalmeiro commented 1 month ago

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

krassowski commented 1 month ago

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).