Extract common CSS vars by gabalafou · Pull Request #14349 · bokeh/bokeh

gabalafou · 2025-02-19T16:35:11Z

mattpap · 2025-03-28T07:21:15Z

+  // attribution is also dark. Not sure what to do about this. Any thoughts?
  background-color: rgba(255, 255, 255, 0.5);
-  font-size: 9px;
+  font-size: 0.5625rem; // 9px if base font is 16px


Wouldn't it be better to configure this as a percentage of the base font?

How would I do that?

If I set this to:

font-size: 56.25%;

I believe that means 56.25% of the font size of the element's parent. And the element's parent may have a different font size than the base font, right? So we need to use rem to specify this as a percentage of the base font, unless I'm misunderstanding something.

I suppose both approaches are valid in certain applications and as long as used consistently. However, given that we indeed define everything in terms of the base font size, then perhaps it would be better to use rem units.

mattpap · 2025-03-28T07:25:41Z

+  // @mattpap: So I think the 0.5 alpha value is to help this look good no
+  // matter what background color is used for the data viz, but this creates
+  // contrast issues when the background is dark, and text color of the
+  // attribution is also dark. Not sure what to do about this. Any thoughts?


Alpha is there to make it see through, so that the annotation doesn't obscure the underlying content too much. I'm not sure what's the best practice to improve contrast issues.

Yeah, I understand why the alpha value is there. I'm just pointing out that it creates the potential for contrast issues that I'm not sure how to solve.

I wonder if there's a better way to implement attributions - specifically drawing it outside of the plot coordinates. I suppose that part of the point of having attribution happen within the plot coordinates is that it becomes more likely to be included if somebody tries to screenshot just the plot and then shares the screenshot — i.e., it's can function like a watermark for the plot.

I can also look into ways to solve this without changing the placement of attributions — maybe text-shadow will solve the contrast issues - not sure, I'll have to investigate.

mattpap · 2025-03-28T07:27:29Z

+  // @mattpap - I don't really like this style of inset focus ring
+  // but I guess this should be tackled in a separate PR?
  outline-offset: -5px;


Yes, as we discussed in the meeting. In fact, please start an issue for this, so that we have a place to discuss possible improvements.

Done!

Accessible focus rings for buttons #14445

mattpap · 2025-03-28T07:29:05Z

+  // to something with better contrast against the lightgray
+  // background color of the dialog header bar. Should we just
+  // change it now?
+  --bokeh-ctrl-color: var(--color);


I would wait with this this for #14415.

mattpap · 2025-03-28T07:31:39Z

+      // rather than use lighten/darken functions because in light
+      // mode we would need to darken but in dark mode we would need
+      // to lighten (I think)
+      background-color: darken(var(--background-color), 10%);


Also we can mix CSS variables with Less functions:

RuntimeError: Error evaluating function `darken`: Argument cannot be evaluated to a color in /home/runner/work/bokeh/bokeh/bokehjs/src/less/dropdown.less on line 42, column 25: 41 // to lighten (I think) 42 background-color: darken(var(--background-color), 10%); 43 }

We may be able to use color-mix() CSS function to replace lighten() and darken(), but even then it would be good to define lighter/darker variants somewhere. However, it looks like #14415 provides enough shades to cover these cases.

mattpap · 2025-03-28T09:55:59Z

+  --block-start-border-width: 3px;
+
+  --close-icon-hover-background-color: var(--error);
+  // Should these dimensions be px or rem?


For now we should keep pixels, but eventually replace them with relative units. Can you start an issue for this?

Replace px with relative units #14446

mattpap · 2025-03-28T09:57:10Z

  position: absolute;
-  background-color: white;
+  background-color: var(--background-color);
+  // @matpapp - why are these commented out?


This should have been removed. Originally Panel was meant to be one thing, but ended up being a different kind of component, for which setting default borders and padding doesn't make sense.

mattpap · 2025-03-28T09:58:16Z

+// @mattpap - these colors should probably be parameterized
+// but I think this is pretty much only used by the examiner,
+// so maybe not high enough priority to fix?
 .bk-nullish { color: #7724c1; }
 .bk-token { color: #881280; }
 .bk-boolean { color: #007500; }


You can leave them out for now. These are specific to pretty printing models, properties and values, used primarily in examiner component for the time being.

mattpap · 2025-03-28T10:00:12Z

-  background-color: #5f6368;
+  // @mattpap - same here, we might need something like "muted color"
+  background-color: var(--muted-color);


That should probably be just icon color. Looking at the following lines, this may predate SVG images, so I will definitely want this to get migrated to use icons, so everything else, including relevant colors, will be inherited.

That creates low contrast with the background (2.5:1), so I'd rather not change it to match --icon-color.

mattpap · 2025-03-28T10:05:49Z

+  // @mattpap - I couldn't find a good substitute for this color from the colors
+  // already defined in base.less. We might need something like "muted
+  // background color"
+  background-color: var(--muted-bg-color);
+  border-color: var(--border-color);


This should use the "default" gray (like of widgets' background). The muted appearance comes from opacity: 0.6 for non-dirty properties and if dirty it reverts to 1.0.

.prop > * { opacity: 0.6; } .prop.dirty > * { opacity: 1.0; }

gabalafou

Done self-reviewing.

@mattpap, I think I addressed all of your comments and concerns, this is ready for another round of review.

gabalafou · 2025-04-02T18:43:35Z

+  // attribution is also dark. Not sure what to do about this. Any thoughts?
  background-color: rgba(255, 255, 255, 0.5);
-  font-size: 9px;
+  font-size: 0.5625rem; // 9px if base font is 16px


How would I do that?

If I set this to:

font-size: 56.25%;

I believe that means 56.25% of the font size of the element's parent. And the element's parent may have a different font size than the base font, right? So we need to use rem to specify this as a percentage of the base font, unless I'm misunderstanding something.

gabalafou · 2025-04-03T00:26:26Z

+  // @mattpap - I don't really like this style of inset focus ring
+  // but I guess this should be tackled in a separate PR?
  outline-offset: -5px;


Done!

Accessible focus rings for buttons #14445

gabalafou · 2025-04-03T00:49:51Z

+  --block-start-border-width: 3px;
+
+  --close-icon-hover-background-color: var(--error);
+  // Should these dimensions be px or rem?


Replace px with relative units #14446

gabalafou · 2025-04-03T01:18:24Z

+  // @mattpap: So I think the 0.5 alpha value is to help this look good no
+  // matter what background color is used for the data viz, but this creates
+  // contrast issues when the background is dark, and text color of the
+  // attribution is also dark. Not sure what to do about this. Any thoughts?


Yeah, I understand why the alpha value is there. I'm just pointing out that it creates the potential for contrast issues that I'm not sure how to solve.

I wonder if there's a better way to implement attributions - specifically drawing it outside of the plot coordinates. I suppose that part of the point of having attribution happen within the plot coordinates is that it becomes more likely to be included if somebody tries to screenshot just the plot and then shares the screenshot — i.e., it's can function like a watermark for the plot.

I can also look into ways to solve this without changing the placement of attributions — maybe text-shadow will solve the contrast issues - not sure, I'll have to investigate.

gabalafou · 2025-04-03T01:33:11Z


  --bokeh-top-level: 10000; // used for z-index of menus, dropdowns, etc.

+  --color: #2f2f2f;


Got this off-black color from tooltip text. Eventually, will pull in the gray-900 from #14415.

In this PR, everywhere that was using pure black for text color, I replace with this off-black via var(--color).

gabalafou · 2025-04-03T01:42:57Z

+  --disabled-background-color: #eee;
+
+  --input-focus-border-color: #66afe9; // rgb(102, 175, 233)
+  --input-focus-halo-color: rgba(102, 175, 233, 0.6); // fade(#66afe9, 60%)


The set of variables in this file (base.less) is almost exhaustive with respect to this PR but not quite. In particular, in buttons.less and tables.less, there are a number of variables defined in those two files that I chose not to hoist up to this file for now.

The way I see it, in the next PR, I will define all of the colors from #14415 in base.less and use them in buttons.less and other places.

gabalafou · 2025-04-07T19:12:07Z

  gap: 1em;
  padding: 5px;
-  background-color: lightgray;
+  background-color: var(--border-color);


As I review my own code, I notice that it looks odd to to set background-color to var(--border-color). However, one could think of the dialog header as a very thick border. But maybe this should be hoisted up at least to the top of the file (as --bokeh-header-bg-color), maybe to base.less. What do you think?

gabalafou · 2025-04-07T19:14:26Z

+  --block-start-border-width: 3px;
+
+  --close-icon-hover-background-color: var(--error);
+  // Should these dimensions be px or rem?


mattpap · 2025-04-16T06:34:12Z

Baseline changes are limited in scope and trivial. There's one thing, however, that I don't like, which are inner shadows in buttons:

old
new

My preference is to keep the original or closer to it.

mattpap · 2025-04-16T07:02:35Z

This looks good. It doesn't seem to be breaking anything. Baseline changes are trivial (need updating).

gabalafou · 2025-04-23T12:24:32Z

I updated all of the image snapshots and removed all of the "@mattpap" comments or replaced them with "TODO" comments.

mattpap

This looks good as the first iteration. No major changes in baseline images.

gabalafou · 2025-04-28T07:22:18Z

Thanks @mattpap I'll merge this into branch-3.8 after the sync on Wednesday

mattpap · 2025-04-28T07:54:34Z

I'll merge this into branch-3.8 after the sync on Wednesday.

That's a good idea. I was going to suggest anyway, to not merge immediately and give other time to process this, especially given that this was marked as ready not too long ago. I would suggest to mark PRs as ready as soon as you don't intend to include any major changes (e.g. I often mark as ready before I add tests, though I state that in the PR).

github-actions · 2025-08-13T15:00:59Z

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

gabalafou added status: WIP grant: CZI R6 Funded by CZI Round 6 grant labels Feb 19, 2025

mattpap added this to the 3.8 milestone Feb 19, 2025

mattpap modified the milestones: 3.8, 3.9 Mar 7, 2025

mattpap changed the base branch from branch-3.7 to branch-3.8 March 7, 2025 07:55

gabalafou added 2 commits March 11, 2025 13:52

Tabs CSS vars

3aad561

check in

1661ab5

gabalafou force-pushed the gabalafou/css-vars-tabs branch from ef97781 to 1661ab5 Compare March 19, 2025 15:33

gabalafou marked this pull request as draft March 19, 2025 15:34

gabalafou added 5 commits March 24, 2025 21:55

do not rename variables in tooltips.less

c358849

update attribution.less

7d4745f

left question for @matpapp in pretty.less

00552ed

dialogs.less

05ed746

buttons, dialogs, drawers, examiner, panels

8fd32ab

gabalafou changed the title ~~Tabs CSS vars~~ CSS vars Mar 25, 2025

mattpap reviewed Mar 28, 2025

View reviewed changes

gabalafou added 4 commits March 31, 2025 13:50

fix less compilation

1740806

inputs.less

d8e455e

choices.less

eb8a59f

palette_select_item

3902102

gabalafou added 7 commits April 2, 2025 19:35

set color to gray-900

faf0902

remove unused colors from base.less

41d9f25

remove extraneous newline

9b45d67

update comment in base.less

7674ca6

remove comment in buttons.less

4d8a6d0

set dialog top bar icons to same color as before

0aae8c6

remove comment tabs.less

a09a248

gabalafou commented Apr 7, 2025

View reviewed changes

gabalafou marked this pull request as ready for review April 7, 2025 20:28

surface-background-color

02ddb6a

update less files

0b60499

gabalafou changed the title ~~CSS vars~~ Extract common CSS vars Apr 23, 2025

update test image baselines

9a2cea4

gabalafou mentioned this pull request Apr 23, 2025

Bring in color system #14467

Merged

3 tasks

mattpap added status: ready and removed status: WIP labels Apr 27, 2025

mattpap modified the milestones: 3.9, 3.8 Apr 27, 2025

mattpap approved these changes Apr 27, 2025

View reviewed changes

gabalafou merged commit be6a7a7 into branch-3.8 May 5, 2025

gabalafou deleted the gabalafou/css-vars-tabs branch May 5, 2025 13:55

mattpap added status: accepted and removed status: ready labels May 5, 2025

github-actions Bot locked as resolved and limited conversation to collaborators Aug 13, 2025


		--bokeh-top-level: 10000; // used for z-index of menus, dropdowns, etc.

		--color: #2f2f2f;

Uh oh!

Uh oh!

Conversation

gabalafou commented Feb 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gabalafou left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattpap commented Apr 16, 2025

Uh oh!

mattpap commented Apr 16, 2025

Uh oh!

gabalafou commented Apr 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mattpap left a comment

Choose a reason for hiding this comment

Uh oh!

gabalafou commented Apr 28, 2025

Uh oh!

mattpap commented Apr 28, 2025

Uh oh!

github-actions Bot commented Aug 13, 2025

Uh oh!

Reviewers

gabalafou commented Feb 19, 2025 •

edited

Loading

gabalafou commented Apr 23, 2025 •

edited

Loading