feat(widgets) theme widget applies styles to widget container#9558
feat(widgets) theme widget applies styles to widget container#9558chrisgervang merged 13 commits intomasterfrom
Conversation
chrisgervang
changed the title
There was a problem hiding this comment.
Pull Request Overview
This PR follows up on issue #9471 by enhancing widget theme support and updating default styles.
- Adds new CSS variable definitions (e.g. '--widget-margin') and converts hex color values to RGB for consistency.
- Updates the ThemeWidget default id and adjusts the mechanism for applying theme styles to the widget container.
- Revises documentation to clarify widget id requirements and styling details.
Reviewed Changes
Copilot reviewed 12 out of 13 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| modules/widgets/src/themes.ts | Updated CSS variable definitions and theme colors now using RGB values. |
| modules/widgets/src/theme-widget.tsx | Changed default widget id and modified theme style application logic. |
| modules/widgets/src/compass-widget.tsx | Updated compass colors to use RGB values. |
| modules/core/src/lib/widget-manager.ts | Added 'deck-widget-container' class to the parent element and removed the 'deck-theme' class from view containers. |
| Docs Files | Updated widget documentation to reflect new defaults and style application. |
Files not reviewed (1)
- modules/widgets/src/stylesheet.css: Language not supported
Comments suppressed due to low confidence (2)
modules/widgets/src/themes.ts:35
- The '--icon-compass-south-color' value for LightTheme is 'rgb(204, 204, 204)', while DarkTheme uses 'rgb(200, 199, 209)'. Please confirm that this difference is intentional and aligns with the design specifications.
'--icon-compass-south-color': 'rgb(204, 204, 204)'
modules/core/src/lib/widget-manager.ts:206
- Removing the 'deck-theme' class from viewContainer may affect any CSS or JS that targets it. Ensure that any dependencies on this class are updated accordingly.
viewContainer.classList.add('deck-theme');
|
|
||
| const el = this.element; | ||
| if (el) { | ||
| const container = el.closest('.deck-widget-container'); |
There was a problem hiding this comment.
If a '.deck-widget-container' is not found, the theme style will not be applied. Consider adding a fallback mechanism or logging a warning for easier debugging.
ibgreen
left a comment
ibgreen
left a comment
There was a problem hiding this comment.
Good to see this cleanup. Adding some comments mostly to illustrate how I think about these things, as usual feel free to ignore if not aligned with your direction.
|
|
||
| new Deck({ | ||
| theme: mode === 'dark' ? DarkTheme : LightTheme | ||
| style: mode === 'dark' ? DarkTheme : LightTheme |
There was a problem hiding this comment.
Fine to use this prop, just noting that I can see myself styling deck to fit into my apps HTML, but theming is set separately. I.e. there are two uses of style and we must make sure they work together, or we need to remind the user to always combine theme and app styles in deck.setProps({style})
There was a problem hiding this comment.
We don't need to introduce new concepts for theming widgets - the ideal implementation only uses built-in CSS features keep framework scope to a minimum.
It's true, if a react user chooses to use the style prop, they must remember to combine the theme with their styles - but that's not the best way to apply a theme in my opinion.
It's much easier to apply a theme to :root in their app's stylesheet. And if they switch themes, they can mutate their root with JavaScript.
All we should have to do as a framework is leave the variables undefined, and document what they are.
| @@ -1,15 +1,3 @@ | |||
| .deck-theme { | |||
| --widget-margin: 12px; | |||
There was a problem hiding this comment.
A little sad to see this go. I felt this was a valuable "exposition" of the theme, its capabilities etc.
There was a problem hiding this comment.
A user should be able to learn about capabilities from documentation rather than reading the source.
|
|
||
| const el = this.element; | ||
| if (el) { | ||
| const container = el.closest<HTMLDivElement>('.deck-widget-container'); |
There was a problem hiding this comment.
Nit:
- In terms of naming, I would elevate the theme as something separate from / "bigger than" widgets
- perhaps by calling the class
deck-themeordeck-theme-container.
There was a problem hiding this comment.
Deck doesn't currently have styles outside of widgets. Do you have some in mind?
If we elevated it to the top-level of deck, there's a more more to consider:
- where the CSS class is applied within core. Currently, it's neatly scoped to the
WidgetManagerbut if it's meant to apply to the rest of deck's DOM it should probably be added inDeckinstead. - how it works in environments where widgets aren't supported (MapboxOverlay, GoogleMapsOverlay, etc..)
I'm just thinking if deck doesn't have styles outside of widgets, the next level up would be controlling the application's theme. Then you'd want the widget to behave like a controlled react input (value/onChange props).
| '--button-size'?: string; | ||
| '--button-corner-radius'?: string; | ||
| // inter-icon color | ||
| '--icon-compass-north-color': string; |
There was a problem hiding this comment.
I personally feel that a theme should not have knowledge about a specific widget, rather define some logical colors.
if that is hard to think of, lightly less specific would be to drop the --icon-compass and just use:
--north-color
--south-color
Follow up on #9471. For #9490
Change List
deck-widget-containerCSS class to widget container. By default this is<body>, but user's control it with DOM placement or theparentprop. In react environment's it is within the deck component.