Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[SIP-82] Improving Superset Theming #20159

Closed
michael-s-molina opened this issue May 23, 2022 · 27 comments
Closed

[SIP-82] Improving Superset Theming #20159

michael-s-molina opened this issue May 23, 2022 · 27 comments
Assignees
Labels
sip Superset Improvement Proposal

Comments

@michael-s-molina
Copy link
Member

michael-s-molina commented May 23, 2022

[SIP-82] Improving Superset Theming

Motivation

Recently we did a POC with the objective of changing the colors of the Superset theme to analyze how it affected the application. The reason was to understand how far we are from allowing Superset users to customize the theme. The objective of this SIP is to document the findings of the POC and to propose some efforts to improve Superset theming capabilities. We understand that some of the efforts may need additional SIPs or are linked with existing SIPs. It’s not the objective of this SIP to dive into the details of all the efforts but to propose an overview of the work necessary and to propose solutions to some of them.

The Superset theme configuration has the following structure:

const defaultTheme = {
  borderRadius: 4,
  colors: {
    text: {...},
    primary: {...},
    secondary: {...},
    grayscale: {...},
    error: {...},
    warning: {...},
    alert: {...},
    success: {...},
    info: {...},
  },
  opacity: {...},
  typography: {
    families: {...},
    weights: {...},
    sizes: {...},
  },
  zIndex: {...},
  transitionTiming: 0.3,
  gridUnit: 4,
};

We started by changing the theme's primary, secondary, and grayscale colors to green, purple, and gold respectively using colors from Ant Design color palettes.

Primary Secondary Grayscale
Screen Shot 2022-03-16 at 1 03 27 PM Screen Shot 2022-03-16 at 1 05 54 PM Screen Shot 2022-03-16 at 1 04 36 PM

We also changed text colors to magenta. Here's the result of these changes on some of our main screens:

Screen Shot 2022-03-16 at 1 15 00 PM

Screen Shot 2022-03-16 at 1 56 09 PM

Screen Shot 2022-03-16 at 1 57 43 PM

Screen Shot 2022-03-16 at 3 07 59 PM

Some interesting conclusions can be drawn from the above images:

  • Some areas of the application are not affected at all by theme changes like the main header, backgrounds, links, and charts
  • Some components are partially affected by the theme changes like the Select
  • There's a clear difference in how the background colors are handled between SQL Lab and the other modules
  • The secondary color is rarely seen
  • Changing the text color to magenta didn't produce any effect
  • Many pages in Settings are rendered on the server-side and are not connected with the theme at all

Proposed Change

To deal with these problems and make Superset themeable, we need to complete some efforts.

Standardize theme access

If we do a quick search for “supersetTheme" we can see that the theme object is accessed directly in some files. This means that these files will not be affected by potential changes to theming properties. At the time of writing, we have 40 files referencing the theme directly. We need to fix these files and standardize theme access.

Get rid of the remaining LESS files

We need to complete our work and remove the remaining LESS files from the application. At the time of writing, we have 17 LESS files remaining. Given that each LESS file contains many style definitions that may or may not be in use by the application, this task will require a great deal of effort and can potentially introduce regressions.

Get rid of the remaining CSS files

Some plugins are using CSS files to configure the theme properties. We need to migrate them to Emotion and make the necessary adjustments to colors, fonts, etc. At the time of writing, we have about 20 CSS files.

Remove hard-coded typography and colors

Many components are still using hard-coded typography (font families, weights, sizes) and colors. At the time of writing, we don't have an estimate on how many components need to be fixed.

Write linting rules to find and warn on non-themed styles we care about in Emotion

One effort that can help us find non-themed styles and prevent future incompatibilities is to write linting rules that are capable of finding these errors. It is worth spending some time trying to make this work because it can really buy us time in the short and long term.

Change the way we import Ant Design from LESS to CSS

Right now we are importing Ant Design LESS files directly to override some of its variables. As we progress with removing LESS dependencies we also need to change the way we import Ant Design to use the CSS version as described in the installation docs.

Migrate server-side pages to the client-side

Many Superset pages are still being rendered on the server-side. Some examples include:

  • Users list
  • Roles list
  • Role Level Security filters
  • Action Log list
  • Info page

Right now these pages aren't affected by theming changes. We considered maintaining these pages on the server-side and try to apply the theme configurations to them. After analyzing the complexities and limitations of the solution, we recommend migrating them to the client-side since this is also a requirement for the single-page application (SPA) effort. As part of this work, we may need new APIs to support/hydrate these pages.

Improve the theme structure definition

The theme structure definition constitutes an API between Superset and theme providers. A poorly defined structure will make it difficult for theme providers to understand which parts of the application are affected by the changes, may limit the areas for customization, and can lead to unnecessary migration while evolving.

Looking at the structure of our current theme, we found that we needed to make some improvements to support multiple themes. To come up with the improvement suggestions we took inspiration from the following references during the research phase:

To support multiple themes, we are suggesting the following changes to the colors theme structure:

colors: {
  background: {
    default: <value>,
    surface: <value>,
    popover: <value>,
  },
  text: {
    label: <value>, // remove
    help: <value>, // remove
    default: <value>,
    hint: <value>,
    disabled: <value>,
  },
  primary: {},
  secondary: {}, // remove
  grayscale: {},
  error: {},
  warning: {}, // make this yellow
  alert: {}, // remove
  success: {},
  info: {},
},

Create a background category with the default, surface, and popover subcategories

We noticed that we need these categories to support the customization of background structures like dashboard cards, popovers, tooltips, navigation panels, etc. These colors are independent of the primary and grayscale categories. This became really clear when simulating a dark theme.

Modify the text category

We need to add more text categories to reflect the different use cases found in the application. The default category is used to display the majority of texts in the application. We also created the hint and disabled categories and removed the label and help categories.

To deal with texts that appear on top of primary and grayscale colors we'll use algorithms to automatically determine the most appropriate color.

Remove the secondary color

Currently, the secondary color is rarely seen in Superset. We're proposing to adjust these rare places to use primary and grayscale colors and remove the secondary category, simplifying the theme configuration.

Merge the warning and alert categories

We can simplify the configuration of our functional colors by merging the warning and alert categories. The idea is to keep the warning category with the colors from the alert category because both the name and the gold colors are most commonly found in other applications and align better with the users’ basic understanding of color functions.

We can also improve typography definitions. Although we have families, weights, and sizes, it's not always clear where they are being used and how they are being compounded to create common text elements. Because the composition is not clearly established, developers tend to mix different weights and sizes for components that should belong to the same typography category.

To improve the standardization of typography categories and at the same time maintain the flexibility of the atomic configurations, we propose to enhance our theme with the introduction of presets. Presets are a combination of families, weights, sizes, and line heights that form the typography building blocks of the application. They are not arbitrarily defined but follow the design system specification which means that any change to them should be a reflection of a change in the design system. One important step we can take to make sure this statement remains true in the future is to explicitly set code owners for the theme files with members of the design team and selected engineers.

Here’s the typography building blocks as defined by the design system (@kasiazjc):

Screen Shot 2022-05-23 at 8 00 03 AM

The proposed changes are to adjust the weights and sizes categories to reflect the design system and to create the presets category:

typography: {
  families: {
    sansSerif: `'Inter', Helvetica, Arial`,
    serif: `Georgia, 'Times New Roman', Times, serif`,
    monospace: `'Fira Code', 'Courier New', monospace`,
  },
  weights: {
    light: 200, // remove
    regular: 400, // normal -> regular
    medium: 500,
    semiBold: 600,
    bold: 700, // 600 -> 700
  },
  sizes: {
    xxs: 9,  // remove
    xs: 10, // remove
    s: 12,
    m: 14,
    l: 16,
    xl: 18, // 21px -> 18px
    xxl: 24, // 28px -> 24px
  },
  presets: {
     heading1: {
        fontFamily: families.sansSerif,
        fontWeight: weights.medium,
        fontSize: sizes.xxl,
        lineHeight: 1.3,
     },
     bodyDefault: {
        fontFamily: families.sansSerif,
        fontWeight: weights.normal,
        fontSize: sizes.m,
        lineHeight: 1.3,
     },
     monospace: {
        fontFamily: families.monospace,
        fontWeight: weights.normal,
        fontSize: sizes.s,
        lineHeight: 1.3,
     },
     ... // remaining presets
  }
}

With these definitions, we can easily map them to the screens and maintain a consistent design throughout the application:

Dashboards

The suggested changes here constitute the first draft for the theme categories and are not complete. As this is an iterative approach, we’re open to further modifications to the theme structure while working on theming tasks. We can use the dark theme implementation as the compass to define if we need additional categories or if further simplifications can be made.

Define a way to apply our theme to Ant Design's theme

One problem we currently have is how to customize Ant Design's theme directly. Right now we have many components that override the native styles by changing Ant Design CSS properties manually. Some components are extensions to the native components and add new features or modify the way a component behaves. Others are simply an extra layer to change CSS. The problem with that approach is that it increases the probability of breaking changes when upgrading Ant Design versions. One of the bigger problems right now is that Ant Design uses LESS for defining theme variables and the support for dynamically changing these variables is really poor. Luckily, after many requests from the community, the Ant Design team finally decided to support CSS-in-JS in Ant Design 5. This will probably change the way we are modifying native styles and reduce the number of custom components on our end. We don't have an estimate for this version yet so we need to come up with a temporary solution. During the POC we come up with three different possibilities:

Upgrade Ant Design to the latest version and use CSS variables

One possibility is to use the new CSS variables feature available in the latest version of Ant Design. This feature allows us to dynamically override theme properties using a ConfigProvider. The downside of this feature is that only some properties are actually customizable, which means this solution may not be sufficient for our use case. We would also need to upgrade Ant Design to the latest version and that would require adjustments to our application to make sure the styles are still working. The advantage of this approach is that we won't expand our layer of custom components, preventing work that may be discarded when upgrading to Ant Design 5. Also, it would be a catalyst to finally get visual testing integrated into our development environment. Migrating to the latest version would also help prepare the code base for the next major version upgrade.

Complete the layer of custom components

Another possibility is complete our layer of custom components by adding mappings to Ant Design components that are currently being accessed directly. This has the disadvantage of expanding our custom layer even more which may change after upgrading to Ant Design 5. The advantages are that we don't need to upgrade Ant Design, avoiding all the migration errors, and the level of possible customization.

Wait for Ant Design version 5

We can also wait for the next major version and limit the scope of what we can customize. During the POC it was hard to determine if the areas that were not affected by theming changes are the result of code that is not using the theme correctly or if it's related to Ant Design styles.

Make use of global styles to override specific components

We could also use global styles to fill in the blanks and override specific components while keeping the Ant Design version. This could be a temporary patch until we wait for version 5.

Conclusion

I think the decision of which approach to follow should be made after we fix incorrect code and check the remaining non-themeable parts of the application. Then we can fully determine which approach is faster, given that the new Ant Design version will probably change the solution.

Having visual regression testing enabled for this work would be essential to mitigate potential regressions. We should make it a pre-requirement for many of the efforts described here.

Apply our theme to ECharts plugins

ECharts supports theme customization by providing and registering a theme object or using a predefined theme and explicitly setting style options.

Here's a preview of the ECharts theme structure obtained by downloading the dark theme:

var colorPalette = ['#dd6b66','#759aa0','#e69d87','#8dc1a9', ...];
var theme = {
   color: colorPalette,
   backgroundColor: '#333',
   tooltip: {...},
   legend: {...},
   textStyle: {...},
   title: {...},
   toolbox: {...},
   dataZoom: {...},
   timeline: {...},
   timeAxis: {...},
   logAxis: {...},
   valueAxis: {...},
   categoryAxis: {...},
   line: {...},
   graph: {...},
   gauge: {...},
   candlestick: {...}
};

In our case, we recommend using the default theme and mapping our theme configurations to each category defined by ECharts. Currently, each plugin is passing these ECharts configurations via an options object that is calculated by the transformProps and loadTransformProps functions. Right now, each plugin is creating its own version of the options object. To allow customization of properties that are applied to all plugins we can initialize the Echart component with a base options object and complement its properties with the ones coming from the plugins. We also need to remove base configurations from the plugins unless we're specifically overriding the behavior. Here's an example:

// src/components/Echart.tsx
function Echart({ echartOptions, ... }: EchartsProps) {
   ...
   const theme = useTheme();
   const chartRef = useRef();
   const option = {
      backgroundColor: theme.colors.background.default,
      textStyle: {
         ...theme.typography.presets.default,
      },
      ...echartOptions,
   };
   chartRef.current.setOption(option, true);
}

// src/Radar/transformProps.ts
export default function transformProps(theme, ...) {
   const echartOptions: EChartsCoreOption = {
      ...,
      radar: {
         splitLine: {
            lineStyle: {
               color: [theme.colors.grayscale.base],
            },
         },
      },
   };
   return {echartOptions, ...};
}

As you can see from the examples, we need to make the Superset theme available for both functions.

Apply our theme to non-ECharts plugins

To apply our theme to plugins that are not using the ECharts library, we'll need to evaluate each plugin individually and determine what's the best approach, whether is to manually apply CSS classes to the relevant parts of the plugin (e.g. Pivot Table) or use the plugins' API to change theming properties (e.g. Word Cloud).

Create a dark theme to test the application

Creating a dark theme will be a good test for Superset themeability. Using a theme with opposite colors enables us to validate if all UI elements are being affected by theming changes. This is especially useful to check text, borders, shadows, and background colors with different contrasts. We could also make the dark theme available as one of the default themes after the work is completed.

Create a wiki page on how to customize Superset

Organizing the theme structure helps a lot but there's nothing better than a proper theming guide for Superset users. We're suggesting a wiki page with all the information about theme categories, which parts of the application are affected by each category, instructions on how to manage themes, etc.

Future work

In the future, we could extend the work to allow administrators to manage multiple themes via a specific module in Superset. We could have features similar to ECharts theme builder or Material UI theme creator for editing theme configurations. We could also allow administrators to export/import themes to promote theme sharing.

New or Changed Public Interfaces

  • We’re suggesting changes to the theme object definition, which will impact multiple React components
  • Migrating the server-side rendered pages may result in endpoint changes. This will probably be defined in its own SIP
  • The plugins will receive access to the theme object as an additional property

New dependencies

Depending on the output of our research on how to apply our theme to Ant Design, we may have to upgrade its version.

Migration Plan and Compatibility

No database migrations or updates to stored URLs are necessary.

Rejected Alternatives

  • Keep theming variables in CSS or LESS files. We want to leverage Emotion and its dynamic features.
  • Remove the atomic typography categories and keep only the presets. We don’t want to remove the developer’s flexibility as new categories may emerge.
  • Keep server-side rendered pages and make some sort of theming sync. After analyzing the complexities and limitations of the solution, we decided to migrate them to the client-side since this is also a requirement for the single-page application (SPA) effort.

Special Thanks

Special thanks to @kasiazjc @geido @rusackas @mihir174 @jess-dillard @villebro and all the other reviewers for helping with the work.

@ktmud
Copy link
Member

ktmud commented May 23, 2022

Thanks for the comprehensive writeup. This is awesome!

@kasiazjc
Copy link
Contributor

@michael-s-molina this is fantastic, so much great info and details in one big SIP ❤️ yay for Superset's UI consistency and flexibility!

@suddjian
Copy link
Member

suddjian commented May 23, 2022

Great work!

Many components are still using hard-coded typography (font families, weights, sizes) and colors. At the time of writing, we don't have an estimate on how many components need to be fixed.

Could test this much like the color tests, by changing all the theme's font sizes to zero. Anything visible is not using theme variables.

Upgrade Ant Design to the latest version and use CSS variables

This option seems the best to me. We may also benefit from some new components Ant has added.

Is runtime adjustable theming a planned feature for Ant 5?

@villebro
Copy link
Member

Upgrade Ant Design to the latest version and use CSS variables

This option seems the best to me. We may also benefit from some new components Ant has added.

We're actually in a bad place right now with our current version of AntD (4.9.4):

  • While our current version of AntD appears to work fine with React 16 (we're currently at 16.3.1), it no longer formally supports React 16. So we need to bump React first, and that will require figuring out what to do with all the dependencies we have that haven't yet introduced explicit support for React 17+. So this in itself is a fair bit of work.
  • Our current version of AntD over 18 months old, and bumping it to the current version will likely introduce a fair amount of breaking changes. I remember testing Superset with a more recent version a year ago or so, and it completely broke a lot of components. So this will also need to be done carefully to not introduce regressions.

@ktmud
Copy link
Member

ktmud commented May 24, 2022

Maybe this is too much to ask, but if upgrading AntD turned out to be too large an undertaking or if we have to write a lot more wrapper components, can we also consider migrating Superset to a more flexible and less opinionated UI library such as Chakra UI? I used Chakra in some other projects and was completely in love.

Styled-component is a first-class citizen in Chakra. It already uses Emotion under the hood. Inline style props makes it very easy to override ad-hoc styles without bloating shared components. Customize theme with "variant" and "size" and multipart components provides a principled way of achieving visual consistency across components. Theming with Chakra really is a bliss and is light-year ahead of the current experience with AntD. Try it out yourself if you don't believe me!

I know migrating to yet another UI library is a risky bet, but since we are on the verge of making a large investment in CSS/styling again, maybe it's worth at least doing some investigation on these other choices out there?

@villebro
Copy link
Member

Maybe this is too much to ask, but if upgrading AntD turned out to be too large an undertaking or if we have to write a lot more wrapper components, can we also consider migrating Superset to a more flexible and less opinionated UI library such as Chakra UI? I used Chakra in some other projects and was completely in love.
[...]
I know migrating to yet another UI library is a risky bet, but since we are on the verge of making a large investment in CSS/styling again, maybe it's worth at least doing some investigation on these other choices out there?

I'm not a huge fan of big migrations, especially given the fact that we fairly recently migrated to AntD, but some thoughts on this:

  • While using AntD, I sometimes get the feeling it's slightly unstable, with new releases introducing as many regressions as they fix (sound familiar? 😬 ). If I could choose, I'd rather go with a core UI library that's more stable, even if it introduces new features less frequently. If Chakra UI is perceived to be more stable, this would probably be a good selling point.
  • Comparing 1 month GitHub insights for Chakra UI and AntD, I get the feeling Chakra is largely a 1+ person show compared to AntD (3.5 person show by the looks of it). Also the npm downloads seem to indicate a similar difference, with AntD being 2.5 times more popular in terms of downloads. While these are circumstantial metrics at best, they do make me wonder how comprehensive Chakra UI is vs AntD in terms of features.
  • As part of the AntD migration, we've gone a long way in terms of removing direct dependencies on AntD in the Superset frontend codebase. While many of our components probably have props that are heavily influenced by their AntD implementations, I think we're currently better equipped to swap out the underlying UI implementation library (the storybooks are fairly comprehensive IMO).

@michael-s-molina
Copy link
Member Author

Could test this much like the color tests, by changing all the theme's font sizes to zero. Anything visible is not using theme variables.

This is a nice idea 👍🏼

Is runtime adjustable theming a planned feature for Ant 5?

Yes, it is.

We're actually in a bad place right now with our current version of AntD (4.9.4):

Yes! We have some important upgrades to do with Ant Design and React. Especially because React 18 comes with important performance boosts.

Maybe this is too much to ask, but if upgrading AntD turned out to be too large an undertaking or if we have to write a lot more wrapper components, can we also consider migrating Superset to a more flexible and less opinionated UI library such as Chakra UI? I used Chakra in some other projects and was completely in love.

I think it's a fair ask. As you said, it will depend on the amount of work necessary to achieve themeability. When making this decision, we'll have more information on Ant Design 5 as well, which will help with the evaluation.

@kgabryje
Copy link
Member

Great doc, really comprehensive and easy to read!
I'd like to provide some context regarding planned change of font-weight in the theme - bold: 700, // 600 -> 700. I changed the bold font weight from 700 to 600 when I fixed importing our fonts. As you might remember, we used to import fonts Inter and Fira incorrectly - in fact we used the actual fonts only for regular font weight, the bold fonts were calculated by the browser. After fixing imports of fonts with weight 700, it turned out that they were too thick, while 600 looked more consistent with the previous look.
That being said, changing font weight in the theme from 600 to 700 won't really break anything, because the browser will just fall back to using the imported font weight 600. That's just a matter of consistency between our css and actual imported fonts.

@michael-s-molina
Copy link
Member Author

Great doc, really comprehensive and easy to read! I'd like to provide some context regarding planned change of font-weight in the theme - bold: 700, // 600 -> 700. I changed the bold font weight from 700 to 600 when I fixed importing our fonts. As you might remember, we used to import fonts Inter and Fira incorrectly - in fact we used the actual fonts only for regular font weight, the bold fonts were calculated by the browser. After fixing imports of fonts with weight 700, it turned out that they were too thick, while 600 looked more consistent with the previous look. That being said, changing font weight in the theme from 600 to 700 won't really break anything, because the browser will just fall back to using the imported font weight 600. That's just a matter of consistency between our css and actual imported fonts.

Thanks for the context @kgabryje. The way I'm interpreting @kasiazjc's typography table is that previously defined "bold (600)" fonts will change to "semi-bold (600)" and "bold (700)" will only be used by "Monospace-strong" so we'll preserve the original look.

@ktmud
Copy link
Member

ktmud commented May 24, 2022

I understand big migration is a huge risk but since we are talking about theming here, I'll still encourage you all to try out the theming approach in Chakra UI (which is inspired by Theme UI, btw), maybe even just for inspiration of how to apply themes to AntD. It'll blow your mind.

@cdmikechen
Copy link
Contributor

cdmikechen commented May 25, 2022

Color and font is an intuitive change. However, most adjustments based on color and font are not ideal. Can we provide more advanced style changes?
Our company did a lot of work on the adjustment of superset style last year (but there were some hard coding to solve the limitations), and achieved the following results (this was the result of modification on superset and we didn't make a new page):

image

I know we should step by step. And I think the complete function of theme change (especially in China or else, which has some extreme demand for UI) is supported to such a level.

@michael-s-molina
Copy link
Member Author

This is really cool! Thanks for sharing this @cdmikechen.

I totally understand the need to support advanced customizations. Think of this SIP as the first chapter of a book about Theming 😉. Before adding advanced features, we need to fix our foundation, but eventually, we'll get there! 🚀

@catadedu
Copy link

This is major improvement!!! Any estimates when this will be ready?

@rusackas
Copy link
Member

@catadedu the project represents a whole pile of "long tail" projects, so I think "done" is a bit of a way off. I'll try to detail the current state of affairs in a blog post since a lot of people have been asking some version of this question in various places. I think we're close to the React 18 prerequisite, which is one of many steps, for example.

@rusackas rusackas moved this from [RESULT][VOTE] APPROVED to In Development in SIPs (Superset Improvement Proposals) Oct 3, 2023
@john-bodley john-bodley changed the title [SIP-82] - Improving Superset Theming [SIP-82] Improving Superset Theming Nov 9, 2023
@mistercrunch
Copy link
Member

This is awesome @michael-s-molina. Somehow I missed this SIP when it first came out, and since then, but happy to see this and still think theming / "themability" is a super important topic. One question: say if I was to go all out on making Superset themeable, where would I start?

Thinking about it, maybe I'd go full on brute-force using a dark-theme overriding defaultTheme and fix the most shocking/visible things while browsing/clicking around the app? Start from the top of the proverbial "cascade" (as in the C of CSS) and work my way down into more specific components?

Another approach would be to operate based on the design system we kind of have, and work on a component-by-component basis while using react-storybook. That way you can kind of break it down and into 1) making the design system fully themeable, and 2) which is a larger challenge -> making sure the design system is used everywhere in the app

@mistercrunch
Copy link
Member

I'm still thinking about this after my previous comment! But an interesting question is - once we'd get to a fully themed/themable Superset, how do maintain this? How do we prevent regressions? This potentially adds a super complex dimension to the test matrix. It seems that making sure that all pieces of the app are fully themed and remain that way might be super challenging.

@rusackas
Copy link
Member

rusackas commented Feb 6, 2024

There's actually a blog post on the topic too. There are a few "first things" to tackle, actually... I have a theory about how we can smoothly upgrade AntD, which is one of them... but that's dependant on rekindling visual regression testing... which is dependent on storybook... which is in the midst of a hairy upgrade. I have the feeling we'll find our way out of some of these logjams soon though.

@michael-s-molina
Copy link
Member Author

One question: say if I was to go all out on making Superset themeable, where would I start?

I would start with typography and making sure we can change it consistently throughout the application. This has the benefit of layering down the foundation for other steps, improves text standardization, and it's the shortest checkpoint that will already provide value.

Thinking about it, maybe I'd go full on brute-force using a dark-theme overriding defaultTheme and fix the most shocking/visible things while browsing/clicking around the app?

I thought about the same strategy to validate the colors.

But an interesting question is - once we'd get to a fully themed/themable Superset, how do maintain this? How do we prevent regressions?

As @rusackas mentioned, visual regression testing might be the way to go here.

@mistercrunch
Copy link
Member

@rusackas do you think that a key part of the strategy would be done through react-storybook ? Seems if we set up some themePresets at that level, and make that a construct in storybook, people can do a lot of work simply on these grounds.

Maybe the way we fix higher level components that are NOT in the design system is by creating theme-compatible higher level components that are theme-compatible.

@rusackas
Copy link
Member

Yes, absolutely, Storybook can be used exensively to facilitate the transition. First, it will allow us to make the upgrade to AntD more reliably (checking/testing individual component features, and using Applitools to check for visual regressions). Then, once AntD is upgraded and more theming-compatible, we can put test themes in Storybook (e.g. Dark Mode) and look for problems with each and every core component.

@mistercrunch
Copy link
Member

mistercrunch commented Dec 27, 2024

hey - excuse the robotic tone in the long comment bellow - I used GPT to help organize my thoughts:

So I’ve been deep-diving into theming recently and have managed to fully (95%+) theme Superset in dark mode. This exploration has brought up some insights and recommendations I’d like to share.

Context and Findings

  • Ant Design v5 Theming System: Since the SIP was authored, Ant Design (antd) v5 introduced a rich and extensible theming system. It includes a sensible set of tokens, rich semantics for defining themes, aliases, algorithms (e.g., darkAlgorithm, defaultAlgorithm, compactAlgorithm), and an interactive theme editor. It seems to offer everything needed for rich, dynamic theming.

  • Overlap and Maturity: Our current theming object overlaps significantly with what antd’s theming system exposes but lacks its maturity and extensibility.

  • Limitations of Mapping Themes: Maintaining a mapping between our theme’s data structures and antd’s is proving to be limiting, reductive, and error-prone as we adopt antd v5 components.

Recommendation

I recommend fully embracing antd’s theming system as the foundation for Superset theming, extending it through their APIs. Here’s why:

  • Alignment with Vanilla Antd Components: By adopting antd’s theming system directly, we can use vanilla antd v5 components throughout the app without maintaining mappings.

  • Rich Tooling for Users/Admins: Users/admins can leverage antd’s powerful tooling and APIs to define Superset themes. This includes token semantics, component-specific styling, and algorithmic controls. They can even create custom algorithms compatible with antd and compose them flexibly.

  • Enhanced Token Semantics: For non-antd components, we’d gain access to antd’s rich token system in emotion. Instead of using generic tokens like theme.colors.primary.light3, we could adopt descriptive tokens like colorBgPrimary or colorPrimaryBorder that align with modern theming semantics. Our current “light/dark” semantics make less sense in a dark theme context, where antd’s approach is more intuitive and effective.

  • Ecosystem Benefits: Ultimately, our theming objects and abstractions, if done right, would converge towards resembling antd’s system. By embracing their framework, we immediately benefit from the ecosystem they’ve built.

Transition Plan

The transition involves several moving pieces, as detailed in the SIP. I’ve made progress in adapting our current theming to prepare for this shift:

  • Templating Legacy .less Files:

    • I’ve introduced templating for our .less files using Handlebars templates (e.g., .less.hbs). These templates reference the current theme object and dynamically generate .less files.
    • This enables centralized theme management. Modify the theme definition, and the .less files can be re-synced with npm run compile-less. For now, this process is manual, but it’s possible to automate this in the build pipeline (e.g., during npm run build) and even .gitignore the compiled .less files.
  • New Theme Package: Centralized under @superset-ui/core/theme, it:

    • Acts as a compatibility layer between our current theme object and antd v5 themes.
    • Can initialize themes using either a legacy supersetTheme or an antd ThemeConfig object.
    • Enriches the current theme object to include an antd key, exposing all antd tokens for use in emotion. For example, instead of theme.colors.primary.dark4, you can use theme.antd.colorPrimaryBg.
    • Exposes a comprehensive emotion ThemeProvider and an AntdThemeProvider for use in apps or tools like Storybook.
  • Moving from Current Theme Object: Currently, theme.colors.* is referenced 1,042 times in the codebase. Transitioning these to theme.antd.* will represent significant work, but many of these references may become unnecessary as we adopt vanilla antd v5 components.

  • Dropping Theme Mappings: The ongoing antd v5 migration currently maps our theme object to antd’s system. This will no longer be necessary once we fully adopt antd’s theming. Our wrapper layer for antd components can be stripped of styles, allowing native antd theming to take over.

Workload estimates

  • moving to antd-v5: clearly there's a lot of work here, but if we commit to going vanilla antd (so that it can be themed nicely using antd-semantics through a ThemeConfig object), things should be pretty smooth. Part of the hard work around the migration is mapping our current theme object into the props of each component. If we can commit to go vanilla, the migration should be much easier
  • ripping out old less: also a fair amount of work, but as the app becomes a composite of antd component throughout, there should be less and less (pun intended) dependency on less
  • removing references to legacy theme: it's all about going vanilla, ripping out custom styling as we move to antd-v5. For more complex objects like ExploreView, DashboardView or SQLLab, there's still some styling needed, but building upon vanilla antd-v5 component should remove much of the reliance on custom styling.

Conceptual model for the transition

Screenshot 2024-12-27 at 5 21 32 PM Screenshot 2024-12-27 at 5 21 27 PM Screenshot 2024-12-27 at 5 21 42 PM

Final Thoughts

Embracing antd’s theming system positions Superset for a more robust and extensible theming future. While there are challenges in transitioning, the benefits far outweigh the costs. By aligning with antd’s ecosystem, we gain consistency, reduce maintenance burden, and empower users with best-in-class theming tools.

@mistercrunch
Copy link
Member

mistercrunch commented Dec 27, 2024

screencap here walking through the work so far -> https://drive.google.com/file/d/1KKQf6qH70dKhSmZEMBAL5TEK71vA7H_k/view?usp=sharing

some eye candy ...
Screenshot 2024-12-27 at 5 33 35 PM
Screenshot 2024-12-27 at 5 32 41 PM
Screenshot 2024-12-27 at 5 33 46 PM
Screenshot 2024-12-27 at 5 34 24 PM

@michael-s-molina
Copy link
Member Author

@mistercrunch I'm fully supportive of moving towards vanilla Ant Design components. I completely agree that our customizations have a high maintenance cost. @geido and I discussed this problem many times and we reached the same conclusion. I believe it's important to get @kasiazjc's approval as well given that we'll have more limitations in terms of design by following vanilla Ant Design (which I think it's a good thing).

@mistercrunch
Copy link
Member

I'll keep pushing in this direction in #31590, which is pretty major step in that direction and viable pretty much as-is (I just need to fix the cypress tests I've broken along the way 😰). I'll add more notes in that PR, but I added a dark/light theme switcher in the navbar (behind a feature flag) in there which makes it easy clear what's dynamic css-in-js and what isn't. It feels great to rip all that less and css-in-js and go vanilla.

I'd say the goal is for a Superset theme object to effectively be an antd ThemeConfig, augmented with the few things that are important to Superset's theming, yet out-of-scope for something as generic as AntD. Things like brandLogoUrl or it appears that AntD's config object is customizable enough to register our own tokens and components.

@villebro
Copy link
Member

villebro commented Jan 6, 2025

I'd say the goal is for a Superset theme object to effectively be an antd ThemeConfig, augmented with the few things that are important to Superset's theming, yet out-of-scope for something as generic as AntD. Things like brandLogoUrl or it appears that AntD's config object is customizable enough to register our own tokens and components.

One important thing to remember is we will also need to provide a theme object for ECharts. So if we're confident the AntD ThemeConfig can serve as a superset of all required theming functionality, I think this makes sense. However, if this is not the case, we're probably better off just having our own theme object like we do now, and then shimming that into AntD's and ECharts' (and any other library's) respective theming implemenetations.

@mistercrunch
Copy link
Member

mistercrunch commented Jan 7, 2025

One important thing to remember is we will also need to provide a theme object for ECharts

Totally. From what I see it seems very doable to extend the antd theme system with custom tokens and custom components. From what I see you can totally write a ThemeConfig like this:

const themeConfig = {
 token: {
    primaryColor: 'red',  // this is a base token they use internally in their components
    textColorBase: '#AAA', // another seed token I think
    customGlobalToken: 'blue', // this item isn't in the official list
    hackyNestedObject: {'hello': 'world'}, // this seems to also works, but diverges from their flat approach to tokens. potentially could be used to squish a whole echartOptions-compatible nested structure...
  }
  components: {
    Button: { // an official component
      primaryColor: 'brown', // this is the prescribed way to override a token for a specific component
    },
    ECharts: { // this component isn't a registered antd component but will be made available
      echartSpecificTextSize: '20px',
      textColorBase: '#EEE', //override of textColorBase specific to echarts
    }
  }
}

all of which become available through antd's theme.useToken(), usage similar to emotion's useTheme() (it's actually built using it emotion)

So in any component, you can just:

// retrieving custom token in component module
const { customGlobalToken } = theme.useToken();

// retrieving Echarts specific stuff
const { textColorBase, echartSpecificTextSize } = theme.useToken().componentss.Echarts;

I couldn't find a function that merges/flattens tokens in the context of a specific component say useTokenForComponent('Button'), but it'd be super easy to expose such a function, all it would need to do is retrive flat tokens using useToken(), and override based on the useToken().components config.

@mistercrunch
Copy link
Member

Started this discussion on antd's repo -> ant-design/ant-design#52270

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
sip Superset Improvement Proposal
Projects
Development

No branches or pull requests