feat(widgets): ThemeWidget by ibgreen · Pull Request #9471 · visgl/deck.gl

ibgreen · 2025-02-27T17:57:40Z

Closes #

Background

A new ThemeWidget that switches between light and dark mode.

Change List

Note: Setting the themes using Deck level styles has no effect. My hypothesis is that the theme variables are too nested under specific classes?

coveralls · 2025-02-27T18:06:41Z

coverage: 91.631% (+0.003%) from 91.628%
when pulling bef6261 on ib/theme-widget
into ce6e844 on master.

chrisgervang

Deck's DOM structure can vary depending on the environment, but I'm assuming this is much is always true:

<deck-root <!-- deck's style prop -->>
  <widget-container>
    <view-container <!-- class='dark-theme' is applied here -->>
      <widget <!-- widget's style prop -->/>
    </view-container>
    <!-- other views -->
  </widget-container>
</deck-root>

ThemeWidget is close but is ambiguous about an important behavior: Does the widget's theme apply to all widgets or just the widget's neighboring it within the same view? dark-theme's position in the DOM implies the latter, but querySelectorAll implies the former.

I discuss how either could be implemented in my comments, but I'm in favor of the the simpler direction: apply the theme to all widgets (<widget-container>).

I think it's important that this widget follows CSS cascading norms.. Namely,

deck.setProps({ style: ...}) cannot overwrite what this widget sets, because this widgets are lower in the hierarchy then the deck root.
but a widget's style prop should overwrite what this widget sets. (e.g. new FullscreenWidget({ style: ... }) would overwrite the ThemeWidget's effect on itself.

chrisgervang · 2025-03-27T18:21:48Z

+/**
+ * A Deck Theme is a set of CSS variables that control CSS styling of the official widgets.
+ */
+export type DeckWidgetTheme = {


chrisgervang · 2025-03-27T18:28:31Z

+    this.deck?.setProps({
+      style: this.darkMode ? this.props.darkModeTheme : this.props.lightModeTheme
+    });


Suggested change

this.deck?.setProps({

style: this.darkMode ? this.props.darkModeTheme : this.props.lightModeTheme

});

We shouldn't set deck props in widgets (user's will overwrite it).

We shouldn't set deck props in widgets (user's will overwrite it).

Let's sort that out in a second PR

chrisgervang · 2025-03-27T19:25:51Z

+
+The `id` must be unique among all your widgets at a given time. It's recommended to set `id` explicitly if you have multipe widgets of a given thpe.
+
+#### `placement` (string, optional) {#placement}


Just a note of context: Similar to FullscreenWidget, if we're deciding themes apply to all widgets then we'd omit the viewId prop (leave this as-is).

viewId is on compass and zoom widgets to scope the widget to a specific view, useful to get unique controls when you have a MapView side-by-side with a GlobeView, for example.

Just a note of context: Similar to FullscreenWidget, if we're deciding themes apply to all widgets then we'd omit the viewId prop (leave this as-is).

Yes, I am going with the global theme approach.

(For what its worth. I was initially incorrectly assuming the viewId allowed the user to position the widget in a certain view. Perhaps a potential point of confusion for users.)

viewId does position it, and that could be all it does.

So far though I haven't seen feedback to indicate the need for position global widgets (Fullscreen, Screenshot, and Theme)

chrisgervang · 2025-03-27T19:31:55Z

+    this.element = el;
+    this.update();
+    return el;
+  }


Insert initialTheme's effect in onAdd?

Insert initialTheme's effect in onAdd?

Not sure I understand.

chrisgervang · 2025-03-27T21:57:04Z

+    render(ui, element);
+  }
+
+  setProps(props: Partial<ThemeWidgetProps>) {


If we decide to scope the theme, ThemeWidget#setProps would need to account for the widget moving views. We'd need to unset all of the theme variables on the old view, and apply them in the new one.

Needless to say, it's simpler to just apply themes equally to all widgets.

Needless to say, it's simpler to just apply themes equally to all widgets.

We are going with global themes.

Co-authored-by: Chris Gervang <chrisgervang@users.noreply.github.com>

…eme-widget

Co-authored-by: Chris Gervang <chrisgervang@users.noreply.github.com>

ibgreen

Addressed the comments that do not relate directly to the mechanics of updating the CSS variables.

One change I made is that I moved the documentation of widget specific CSS variables (currently just icons) into each widget doc.

I feel that the variables associated with the theme should just be the general variables.

We can still have CSS variables for each widget to allow some extra control but they should specific to the widget (I would prefer if they were also scoped to the widgets).

ibgreen · 2025-03-28T15:21:31Z

+
+The `id` must be unique among all your widgets at a given time. It's recommended to set `id` explicitly if you have multipe widgets of a given thpe.
+
+#### `placement` (string, optional) {#placement}


Just a note of context: Similar to FullscreenWidget, if we're deciding themes apply to all widgets then we'd omit the viewId prop (leave this as-is).

Yes, I am going with the global theme approach.

(For what its worth. I was initially incorrectly assuming the viewId allowed the user to position the widget in a certain view. Perhaps a potential point of confusion for users.)

ibgreen · 2025-03-28T15:27:04Z

      viewContainer.style.pointerEvents = 'none';
      viewContainer.style.position = 'absolute';
      viewContainer.style.overflow = 'hidden';
+      viewContainer.classList.add('deck-theme');


But if we instead are deciding we want themes to apply to all widgets, we'd instead add

I will leave this for a second PR as I have additional thoughts on it and focus on getting the rest of this PR clean

ibgreen · 2025-03-28T15:28:27Z

+    this.element = el;
+    this.update();
+    return el;
+  }


Insert initialTheme's effect in onAdd?

Not sure I understand.

ibgreen · 2025-03-28T15:29:07Z

+    render(ui, element);
+  }
+
+  setProps(props: Partial<ThemeWidgetProps>) {


Needless to say, it's simpler to just apply themes equally to all widgets.

We are going with global themes.

ibgreen · 2025-03-28T15:29:41Z

+    this.deck?.setProps({
+      style: this.darkMode ? this.props.darkModeTheme : this.props.lightModeTheme
+    });


We shouldn't set deck props in widgets (user's will overwrite it).

Let's sort that out in a second PR

ibgreen · 2025-03-28T15:32:36Z

+    // Note: deck does not support updating the style property
+    // this.deck?.setProps({
+    //   style: themeStyle
+    // });
+    const elements = document.querySelectorAll('.deck-theme');


I think closest is what we want to use from within a widget.

Agreed but will leave this for second PR to avoid bike shedding here.

…eme-widget

chrisgervang

Stamping so we can experimentally merge this in. To graduate I'll take a pass on CSS mechanics.

chrisgervang · 2025-03-28T16:58:42Z

+.deck-theme {
+  --widget-margin: 12px;
+  --button-stroke: rgba(255, 255, 255, 0.3);
+  --button-corner-radius: 8px;
+  --button-shadow: 0px 0px 8px 0px rgba(0, 0, 0, 0.25);
+  --button-size: 28px;
+  --button-background: #616166;
+  --button-backdrop-filter: unset;
+  --button-inner-stroke: unset;
+  --button-icon-hover: rgb(24, 24, 26);
+}
+


I don't want to maintain this approach. It's another source of truth that can get out of sync, and is redundant to the JS definitions

ibgreen requested a review from chrisgervang February 27, 2025 17:57

ibgreen mentioned this pull request Feb 27, 2025

feat(core, widgets): Add new Deck.widgetTheme prop (WIP, partial implementation for exposition only) #9457

Closed

ibgreen added 2 commits March 1, 2025 07:54

feat(widgets): ThemeWidget

78e0f5d

working

5046db7

ibgreen force-pushed the ib/theme-widget branch from 822c5f0 to 5046db7 Compare March 1, 2025 13:41

ibgreen added 2 commits March 1, 2025 09:46

fix

b55a235

fix

3b6f9ff

ibgreen mentioned this pull request Mar 4, 2025

Tracker: Widgets #9490

Open

62 tasks

chrisgervang reviewed Mar 27, 2025

View reviewed changes

ibgreen and others added 4 commits March 28, 2025 11:33

Address feedback

f86ed39

Apply suggestions from code review

8249e53

Co-authored-by: Chris Gervang <chrisgervang@users.noreply.github.com>

Merge branch 'ib/theme-widget' of github.com:visgl/deck.gl into ib/th…

fc1ae84

…eme-widget

Apply suggestions from code review

64257c5

Co-authored-by: Chris Gervang <chrisgervang@users.noreply.github.com>

ibgreen commented Mar 28, 2025

View reviewed changes

ibgreen added 3 commits March 28, 2025 11:51

wip

281b29b

Merge branch 'ib/theme-widget' of github.com:visgl/deck.gl into ib/th…

57c00d8

…eme-widget

underscore

2414c41

chrisgervang approved these changes Mar 28, 2025

View reviewed changes

chrisgervang reviewed Mar 28, 2025

View reviewed changes

Comment thread modules/widgets/src/stylesheet.css

merge

a41dff5

ibgreen force-pushed the ib/theme-widget branch from 44087a1 to a41dff5 Compare March 28, 2025 20:19

ibgreen added 2 commits March 28, 2025 16:45

wip

aec1eaf

lint

bef6261

ibgreen merged commit 66bedf6 into master Mar 28, 2025
4 checks passed

ibgreen deleted the ib/theme-widget branch March 29, 2025 14:57

chrisgervang mentioned this pull request Apr 1, 2025

feat(widgets) theme widget applies styles to widget container #9558

Merged

	this.deck?.setProps({
	style: this.darkMode ? this.props.darkModeTheme : this.props.lightModeTheme
	});


		The `id` must be unique among all your widgets at a given time. It's recommended to set `id` explicitly if you have multipe widgets of a given thpe.

		#### `placement` (string, optional) {#placement}

Conversation

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

Uh oh!

Background

Change List

Uh oh!

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

Uh oh!

Uh oh!

chrisgervang left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

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!

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!

ibgreen 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!

chrisgervang left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ibgreen commented Feb 27, 2025 •

edited

Loading

coveralls commented Feb 27, 2025 •

edited

Loading