feat: massive theming-related PR #31590

mistercrunch · 2024-12-20T22:32:36Z

Excuse the large PR, but this is fairly tangled up, and working on fixing up theming probably requires some massive PRs as it's really hard to proceed PR-by-PR - especially during the holiday break...

Introducing less handlebars templates

First. Clearly we should move away from less and commit to emotion/antd for theming Now deleting the less files is going to be difficult. In the meantime, I wanted to provide a way for less files to source from the main theme object. I decided to go with handlebars since that should be part of the build process.

Considerations:

this is temporary, goal is to get rid of less altogether. Having this enables layering the theming work, and allows to make the theme DRY, instead of referncing the same colors a bunch of times
eventually could remove the .less files from the repo, and make sure they are dynamically generated on every builds. In the meantime I thought I'd leave them here, and we can instruct people to alter .less.hds files and run npm run compile-less on demand, if/when altering the main theme

Large refactor - what's in this PR?

less templates - as described above
New Theme object and subpackage as commented in the theming SIP
Refactoring/fixing out-of-theme, css-heavy components
- Alert - vanilla is better!
- AceEditor
- EmptyState refactor (that component WAS A MESS, felt almost like on-purpose maze of indirections), fixed the svg so it can receive currentColor as opposed to using an tag - required for making it themable/dark-modable
- ErrorAlert refactor (also a huge mess, simplifying quite a bit here)
Making antd components more vanilla
Getting rid of a bunch of CSS
Start aligning our css-in-js across the app to use the antd theme tokens
storybook improvements
- improving the theme stories

what's NOT in this PR? - yet to come

more theme.antd referencing in emotion
move CSS / LESS deletion
antd v5 migrations
ability to configure an antd ThemeConfig as the theme setup, this will require more moving from legacy theme object to antd

korbit-ai · 2024-12-20T22:32:40Z

Based on your review schedule, I'll hold off on reviewing this PR until it's marked as ready for review. If you'd like me to take a look now, comment /korbit-review.

Your admin can change your review schedule in the Korbit Console

Chiseling at #31590 and bringing what's atomically committable out of there. This simply adds eslint checks to pre-commit. Note that: - it requires having run `npm i` in superset-frontend - it's set up to NOT run in CI as part of the pre-commit validation workflow, since we run eslint more formally in another workflow Why doing this? Currently it's common to forget to run `npm run lint` prior to committing/pushing, so people can waste time waiting for CI to fail where it could be caught easily. It's nice to have pre-commit do the check itself because it will only evaluate the files that have changed, making it much faster than running a full lint run against all files.

…rt/e2e.ts While working on #31590, I noticed that `expect` was not properly imported. It was using it from global for some unknown reason.

Chiseling at #31590 that has gotten big / unruly, in this PR is a refactor of Alert-related components, going vanilla AntD. Also. Deprecating colors.alerts since it's ambiguous/redundant with warning/error and does not exist in antd-v5

mistercrunch · 2025-01-29T20:47:46Z

Thanks for reporting the visual issues, planning on solving the bulk of them (and other ones we find) prior to merging this.

mistercrunch · 2025-01-29T20:47:51Z

~~/testenv up~~
oops I think we're label-based now!

github-actions · 2025-01-29T23:04:08Z

@mistercrunch Processing your ephemeral environment request here. Action: up.

betodealmeida

Beautiful!

betodealmeida · 2025-02-24T16:35:23Z

superset-frontend/.storybook/preview.jsx

-import { supersetTheme, ThemeProvider } from '@superset-ui/core';
-import { AntdThemeProvider } from '../src/components/AntdThemeProvider';
+import { themeObject } from '@superset-ui/core';
+import { theme } from 'src/preamble';


Looks like we don't need theme in this file?

oh weird, guessing eslint might not be covering this folder...

betodealmeida · 2025-02-24T16:44:34Z

superset-frontend/packages/superset-ui-core/src/theme/Theme.tsx

+
+  private static readonly sizeMap: Record<FontSizeKey, string> = {
+    xs: 'fontSizeXS',
+    s: 'fontSizeSM',


Curious why fontSizeSM and not jut fontSizeS. My brain reads this as `Font size: small-medium"

Similar for fontSizeLG instead of just fontSizeL.

It would also align with FontSizeKey.

I hear you, had similar thoughts but decided to commit to the AntD token semantics that are well documented and supported across AntD components and beyond. More on it here -> https://ant.design/docs/react/customize-theme .

Oh I get that we could match their 2 letters code in FontSizeKey, but I think it's defined elsewhere and used across places (didn't grep, but afraid to find I'd have to touch a bunch of files). Could be done as a follow up as this is all still an internal API

betodealmeida · 2025-02-24T16:46:10Z

superset-frontend/packages/superset-ui-core/src/theme/Theme.tsx

+  private constructor({
+    seed,
+    antdConfig,
+    isDark = false,


It might be over-engineering but I was wondering if this just be an enum, mode = ThemeMode.DARK, for example. Then in the future we could have other modes, like high-contrast.

Dark is a pretty funky construct where based on this we pass in an AntD darkAlgorithm object in ThemeConfig. Similarly they offer a compact algorithm that can be composed with darkAlgorithm, so you can do

const themeConfig = { seed, algorithm: [darkAlgorithm, compactAlgorithm] }

Meaning some of these things are not exclusive, which is a bit funky.

The constructor here allows for different combos through static methods, like Theme.fromSeed(seed, isDark) and another Theme.fromAntdConfig(config). We still need isDark to support legacy logic around theme.colors.primary.dark3.

Could simplify/cleanup later, and until the public API settles. Working with all this, I think standardizing with an extended AntD config makes most sense. If you want a dark theme, give me a dark antdconfig....

betodealmeida · 2025-02-24T16:48:17Z

superset-frontend/packages/superset-ui-core/src/theme/Theme.tsx

+    // const mergedTheme = merge({}, this.theme, partialTheme);
+    // const isDark = tinycolor(mergedTheme.colorBgBase).isDark();
+    // const antdConfig = Theme.getAntdConfig(systemColors, isDark);
+    // this.updateTheme(mergedTheme, antdConfig, isDark);


Remove this?

mmmh, yeah that one is really a TODO: allow people to override the theme through THEME_OVERRIDES in config.py. Needs more thinking around how we allow people to set the theme from a config object. A bit of an artifact from previous theme handling, but it remains hooked in from bootstrap_data in src/preamble.ts.

Good to resurface this as it's a pretty important hanging thread in the whole theming saga. Do we want to allow people to set the theme through config? build time? in-UI?

I'm thinking for now I might want to simply call this method setTheme, and pass what's in the config.py/bootstrap_data as a seed. Pure antd config aren't serializable as they can contain algorightm as object reference, which we can't reference from the backend.

Needs more thinking, but I'm thinking I'll probably do something simple so that it's possible to set the theme from config.THEME_OVERRIDES

betodealmeida · 2025-02-24T16:55:04Z

superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberViz.tsx

@@ -374,7 +374,7 @@ export default styled(BigNumberVis)`
      .kicker,
      .header-line,
      .subheader-line {
-        opacity: ${theme.opacity.mediumHeavy};
+        opacity: 60%;


Curious of why we're hardcoding opacities now?

oh that was a bit of a hard slash (machete in the jungle), here's the rationale for slashing/hard-coding it:

The antd theming system doesn't specify presets for opacity, probably because there's no clear aligned standards or design guidelines to follow here. Case-by-case might be better

use/reuse across the codebase was minimal, with hard-coded widespread, token reuse not common

is there real value around having set values? what should they be? does it help to use midHeavy over 80% or 70%. Would a "themer" want to alter these values centrally ,like "oh I want all the mediumHeavy to be a little lighter in my environment"?

betodealmeida · 2025-02-24T17:16:35Z

superset-frontend/src/components/TimezoneSelector/index.tsx

@@ -151,7 +151,7 @@ export default function TimezoneSelector({
  return (
    <Select
      ariaLabel={t('Timezone selector')}
-      css={{ minWidth }}
+      // css={{ minWidth }}


Suggested change

// css={{ minWidth }}

kgabryje · 2025-02-24T18:03:06Z

A few visual bugs that I found while testing:

Weird background behind dropdown triggers (home page, dashboard)

2 primary buttons in edit dashboard modal (and many other places)

Nit but we probably should have a tooltip or an icon in the light/dark trigger
Gray background behind the native filters bar looks kinda weird. Also, the full tab content background should probably be white, and it's gray (on the right of the deckgl chart)

The background behind the copy button is kinda strong

Gray background behind the pagination component. Also, the edit chart button doesnt look like our secondary buttons. The modal header and table header are white, and they were gray before, not sure if that's intentional. Side note - the border radius seems to be bigger than before, is that intentional?

Submenu looks like it's behind the menu instead of in front of it. That however might be related to the recent antd5 migration because I'm seeing the same thing on master

The tooltips are more transparent and they're a bit hard to read when on top of some text

Discard button should be secondary in edit mode, the right panel was white before (I think it looked better)
Gray background behind the update chart button in Explore - is that intentional? Also, the colors in the controls are a bit strong and overwhelming, they were lighter gray before and I think they looked better

3 primary buttons in Save chart modal

Some controls are unreadable in dark mode

maybe we should make the border color lighter? It looks kinda overwhelming in many places, like here in viz type selector

Lots of primary buttons in sql lab, also the ag grid table theme looks a bit different? Filter input looks like its disabled, but its not

msyavuz

There are some nits(leftover comments, naming etc.) but it looks good

msyavuz · 2025-02-26T07:33:46Z

superset-frontend/packages/superset-ui-core/src/theme/Theme.tsx

+    const bg = isDark ? '#FFF' : '#000';
+    const fg = isDark ? '#000' : '#FFF';


Naming here seems a bit off. bg is white when isDark?

msyavuz · 2025-02-26T07:35:40Z

superset-frontend/packages/superset-ui-core/src/theme/index.tsx

+
+const styled = emotionStyled;
+
+// launching in in dark mode for now while iterating


Suggested change

// launching in in dark mode for now while iterating

msyavuz · 2025-02-26T07:49:19Z

superset-frontend/packages/superset-ui-core/src/theme/Theme.tsx

+  }
+}
+
+/* eslint-enable theme-colors/no-literal-colors */


Suggested change

/* eslint-enable theme-colors/no-literal-colors */

msyavuz · 2025-02-26T08:54:20Z

There are also some more visual bugs? on dark mode:

Chart text on Dashboard is black on dark mode
Explore page white bg on bottom
Sqllab tabs have white bg
Some inputs don't change in dark mode which i think is unintentional

Also i think there is a dark mode Superset logo which we can use in dark mode.

pull-request-size bot added the size/XXL label Dec 20, 2024

github-actions bot added dependencies:npm packages preset-io labels Dec 20, 2024

sadpandajoe added the review:draft label Dec 20, 2024

rusackas self-requested a review December 20, 2024 23:27

mistercrunch force-pushed the template_less branch 2 times, most recently from f1c7929 to 43452ff Compare December 24, 2024 04:49

github-actions bot added the plugins label Dec 27, 2024

mistercrunch force-pushed the template_less branch from 97e182d to 0655910 Compare December 28, 2024 01:08

mistercrunch changed the title ~~feat: feat(less): templatize .less files to reference main theme~~ feat: feat(less): massive theming-related PR Dec 30, 2024

mistercrunch changed the title ~~feat: feat(less): massive theming-related PR~~ feat: massive theming-related PR Dec 30, 2024

mistercrunch force-pushed the template_less branch from 1ca8e6e to 6c6532e Compare December 30, 2024 15:43

github-actions bot added the github_actions Pull requests that update GitHub Actions code label Jan 5, 2025

mistercrunch mentioned this pull request Jan 6, 2025

[SIP-82] Improving Superset Theming #20159

Closed

mistercrunch force-pushed the template_less branch 2 times, most recently from 003f992 to 1652452 Compare January 9, 2025 07:07

mistercrunch mentioned this pull request Jan 14, 2025

chore: add eslint to pre-commit hooks #31845

Merged

mistercrunch mentioned this pull request Jan 14, 2025

chore: properly import expect from chai in cypress-base/cypress/support/e2e.ts #31846

Merged

mistercrunch force-pushed the template_less branch from 1652452 to f89a23b Compare January 14, 2025 22:22

github-actions bot removed the github_actions Pull requests that update GitHub Actions code label Jan 14, 2025

mistercrunch mentioned this pull request Jan 15, 2025

chore: refactor Alert-related components #31858

Merged

mistercrunch added the testenv-up label Jan 29, 2025

mistercrunch force-pushed the template_less branch from e0db918 to 2961115 Compare February 2, 2025 10:11

michael-s-molina added the hold! On hold label Feb 3, 2025

mistercrunch force-pushed the template_less branch 2 times, most recently from a879aaf to 9a7d632 Compare February 15, 2025 00:23

mistercrunch force-pushed the template_less branch 2 times, most recently from f529ad5 to 7e56fe5 Compare February 19, 2025 23:34

betodealmeida approved these changes Feb 24, 2025

View reviewed changes

mistercrunch force-pushed the template_less branch from fb1d6da to 443f805 Compare February 24, 2025 17:49

mistercrunch force-pushed the template_less branch 2 times, most recently from 8224662 to 1d52ecb Compare February 25, 2025 17:05

msyavuz approved these changes Feb 26, 2025

View reviewed changes

mistercrunch added 11 commits February 28, 2025 17:00

feat: messing with the theme

00c6acd

remove package.json edits

6f56869

fixes

8d4a01a

fix ChartCard more options dropdown

840575f

fix color split in modals

d3a7695

simplifying

3c2899a

touchups

cefa702

rebase progress

a4cada7

test

c143e4f

progress

299c1e6

make overlay optoinal for MenuDotsDropdownProps

abe8c8d

mistercrunch force-pushed the template_less branch from a713bdf to abe8c8d Compare March 1, 2025 01:34

add missing files

a092ac6

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: massive theming-related PR #31590

feat: massive theming-related PR #31590

mistercrunch commented Dec 20, 2024 •

edited

Loading

korbit-ai bot commented Dec 20, 2024

mistercrunch commented Jan 29, 2025

mistercrunch commented Jan 29, 2025 •

edited

Loading

github-actions bot commented Jan 29, 2025

betodealmeida left a comment

betodealmeida Feb 24, 2025

mistercrunch Feb 24, 2025

betodealmeida Feb 24, 2025

mistercrunch Feb 25, 2025 •

edited

Loading

betodealmeida Feb 24, 2025

mistercrunch Feb 25, 2025

betodealmeida Feb 24, 2025

mistercrunch Feb 25, 2025

betodealmeida Feb 24, 2025

mistercrunch Feb 25, 2025 •

edited

Loading

betodealmeida Feb 24, 2025

kgabryje commented Feb 24, 2025

msyavuz left a comment

msyavuz Feb 26, 2025

msyavuz Feb 26, 2025

msyavuz Feb 26, 2025

msyavuz commented Feb 26, 2025

		const bg = isDark ? '#FFF' : '#000';
		const fg = isDark ? '#000' : '#FFF';


		const styled = emotionStyled;

		// launching in in dark mode for now while iterating

feat: massive theming-related PR #31590

Are you sure you want to change the base?

feat: massive theming-related PR #31590

Conversation

mistercrunch commented Dec 20, 2024 • edited Loading

Introducing less handlebars templates

Large refactor - what's in this PR?

what's NOT in this PR? - yet to come

korbit-ai bot commented Dec 20, 2024

mistercrunch commented Jan 29, 2025

mistercrunch commented Jan 29, 2025 • edited Loading

github-actions bot commented Jan 29, 2025

betodealmeida left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mistercrunch Feb 25, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mistercrunch Feb 25, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kgabryje commented Feb 24, 2025

msyavuz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msyavuz commented Feb 26, 2025

mistercrunch commented Dec 20, 2024 •

edited

Loading

mistercrunch commented Jan 29, 2025 •

edited

Loading

mistercrunch Feb 25, 2025 •

edited

Loading

mistercrunch Feb 25, 2025 •

edited

Loading