betodealmeida commented on code in PR #31590:
URL: https://github.com/apache/superset/pull/31590#discussion_r1968083881
##########
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 }}
Review Comment:
```suggestion
```
##########
superset-frontend/packages/superset-ui-core/src/theme/Theme.tsx:
##########
@@ -0,0 +1,352 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+/* eslint-disable
react-prefer-function-component/react-prefer-function-component */
+// eslint-disable-next-line no-restricted-syntax
+import React from 'react';
+import {
+ theme as antdThemeImport,
+ ThemeConfig as AntdThemeConfig,
+ ConfigProvider,
+} from 'antd-v5';
+import tinycolor from 'tinycolor2';
+
+import {
+ ThemeProvider as EmotionThemeProvider,
+ CacheProvider as EmotionCacheProvider,
+} from '@emotion/react';
+import createCache from '@emotion/cache';
+// import { merge } from 'lodash';
+
+import {
+ AntdTokens,
+ SupersetTheme,
+ allowedAntdTokens,
+ SharedAntdTokens,
+ SystemColors,
+ ColorVariants,
+ DeprecatedColorVariations,
+ DeprecatedThemeColors,
+ LegacySupersetTheme,
+ FontSizeKey,
+} from './types';
+
+/* eslint-disable theme-colors/no-literal-colors */
+
+export class Theme {
+ theme: SupersetTheme;
+
+ private static readonly defaultTokens = {
+ // Default colors
+ colorPrimary: '#20a7c9',
+ colorError: '#e04355',
+ colorWarning: '#fcc700',
+ colorSuccess: '#5ac189',
+ colorInfo: '#66bcfe',
+
+ // Forcing some default tokens
+ fontFamily: `'Inter', Helvetica, Arial`,
+ fontFamilyCode: `'Fira Code', 'Courier New', monospace`,
+
+ // Extra tokens
+ transitionTiming: 0.3,
+ brandIconMaxWidth: 37,
+ fontSizeXS: '8',
+ fontSizeXXL: '28',
+ fontWeightNormal: '400',
+ fontWeightLight: '300',
+ fontWeightMedium: '500',
+ };
+
+ private antdConfig: AntdThemeConfig;
+
+ private static readonly sizeMap: Record<FontSizeKey, string> = {
+ xs: 'fontSizeXS',
+ s: 'fontSizeSM',
Review Comment:
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`.
##########
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%;
Review Comment:
Curious of why we're hardcoding opacities now?
##########
superset-frontend/.storybook/preview.jsx:
##########
@@ -17,8 +17,8 @@
* under the License.
*/
import { withJsx } from '@mihkeleidast/storybook-addon-source';
-import { supersetTheme, ThemeProvider } from '@superset-ui/core';
-import { AntdThemeProvider } from '../src/components/AntdThemeProvider';
+import { themeObject } from '@superset-ui/core';
+import { theme } from 'src/preamble';
Review Comment:
Looks like we don't need `theme` in this file?
##########
superset-frontend/packages/superset-ui-core/src/theme/Theme.tsx:
##########
@@ -0,0 +1,352 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+/* eslint-disable
react-prefer-function-component/react-prefer-function-component */
+// eslint-disable-next-line no-restricted-syntax
+import React from 'react';
+import {
+ theme as antdThemeImport,
+ ThemeConfig as AntdThemeConfig,
+ ConfigProvider,
+} from 'antd-v5';
+import tinycolor from 'tinycolor2';
+
+import {
+ ThemeProvider as EmotionThemeProvider,
+ CacheProvider as EmotionCacheProvider,
+} from '@emotion/react';
+import createCache from '@emotion/cache';
+// import { merge } from 'lodash';
+
+import {
+ AntdTokens,
+ SupersetTheme,
+ allowedAntdTokens,
+ SharedAntdTokens,
+ SystemColors,
+ ColorVariants,
+ DeprecatedColorVariations,
+ DeprecatedThemeColors,
+ LegacySupersetTheme,
+ FontSizeKey,
+} from './types';
+
+/* eslint-disable theme-colors/no-literal-colors */
+
+export class Theme {
+ theme: SupersetTheme;
+
+ private static readonly defaultTokens = {
+ // Default colors
+ colorPrimary: '#20a7c9',
+ colorError: '#e04355',
+ colorWarning: '#fcc700',
+ colorSuccess: '#5ac189',
+ colorInfo: '#66bcfe',
+
+ // Forcing some default tokens
+ fontFamily: `'Inter', Helvetica, Arial`,
+ fontFamilyCode: `'Fira Code', 'Courier New', monospace`,
+
+ // Extra tokens
+ transitionTiming: 0.3,
+ brandIconMaxWidth: 37,
+ fontSizeXS: '8',
+ fontSizeXXL: '28',
+ fontWeightNormal: '400',
+ fontWeightLight: '300',
+ fontWeightMedium: '500',
+ };
+
+ private antdConfig: AntdThemeConfig;
+
+ private static readonly sizeMap: Record<FontSizeKey, string> = {
+ xs: 'fontSizeXS',
+ s: 'fontSizeSM',
+ m: 'fontSize',
+ l: 'fontSizeLG',
+ xl: 'fontSizeXL',
+ xxl: 'fontSizeXXL',
+ };
+
+ private constructor({
+ seed,
+ antdConfig,
+ isDark = false,
Review Comment:
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.
##########
superset-frontend/packages/superset-ui-core/src/theme/Theme.tsx:
##########
@@ -0,0 +1,352 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+/* eslint-disable
react-prefer-function-component/react-prefer-function-component */
+// eslint-disable-next-line no-restricted-syntax
+import React from 'react';
+import {
+ theme as antdThemeImport,
+ ThemeConfig as AntdThemeConfig,
+ ConfigProvider,
+} from 'antd-v5';
+import tinycolor from 'tinycolor2';
+
+import {
+ ThemeProvider as EmotionThemeProvider,
+ CacheProvider as EmotionCacheProvider,
+} from '@emotion/react';
+import createCache from '@emotion/cache';
+// import { merge } from 'lodash';
+
+import {
+ AntdTokens,
+ SupersetTheme,
+ allowedAntdTokens,
+ SharedAntdTokens,
+ SystemColors,
+ ColorVariants,
+ DeprecatedColorVariations,
+ DeprecatedThemeColors,
+ LegacySupersetTheme,
+ FontSizeKey,
+} from './types';
+
+/* eslint-disable theme-colors/no-literal-colors */
+
+export class Theme {
+ theme: SupersetTheme;
+
+ private static readonly defaultTokens = {
+ // Default colors
+ colorPrimary: '#20a7c9',
+ colorError: '#e04355',
+ colorWarning: '#fcc700',
+ colorSuccess: '#5ac189',
+ colorInfo: '#66bcfe',
+
+ // Forcing some default tokens
+ fontFamily: `'Inter', Helvetica, Arial`,
+ fontFamilyCode: `'Fira Code', 'Courier New', monospace`,
+
+ // Extra tokens
+ transitionTiming: 0.3,
+ brandIconMaxWidth: 37,
+ fontSizeXS: '8',
+ fontSizeXXL: '28',
+ fontWeightNormal: '400',
+ fontWeightLight: '300',
+ fontWeightMedium: '500',
+ };
+
+ private antdConfig: AntdThemeConfig;
+
+ private static readonly sizeMap: Record<FontSizeKey, string> = {
+ xs: 'fontSizeXS',
+ s: 'fontSizeSM',
+ m: 'fontSize',
+ l: 'fontSizeLG',
+ xl: 'fontSizeXL',
+ xxl: 'fontSizeXXL',
+ };
+
+ private constructor({
+ seed,
+ antdConfig,
+ isDark = false,
+ }: {
+ seed?: Partial<SupersetTheme>;
+ antdConfig?: AntdThemeConfig;
+ isDark?: boolean;
+ }) {
+ this.updateTheme = this.updateTheme.bind(this);
+ this.SupersetThemeProvider = this.SupersetThemeProvider.bind(this);
+
+ if (seed && antdConfig) {
+ throw new Error('Pass either theme or antdConfig, not both.');
+ } else if (antdConfig) {
+ this.setThemeFromAntdConfig(antdConfig);
+ } else if (seed) {
+ this.setThemeFromSeed(seed || {}, isDark);
+ }
+ }
+
+ static fromSeed(seed?: Partial<SupersetTheme>, isDark = false): Theme {
+ const theme = new Theme({ seed, isDark });
+ return theme;
+ }
+
+ static fromAntdConfig(antdConfig: AntdThemeConfig): Theme {
+ const theme = new Theme({ antdConfig });
+ return theme;
+ }
+
+ private static genDeprecatedColorVariations(
+ color: string,
+ isDark: boolean,
+ ): DeprecatedColorVariations {
+ const bg = isDark ? '#FFF' : '#000';
+ const fg = isDark ? '#000' : '#FFF';
+ const adjustColor = (c: string, perc: number, tgt: string): string =>
+ tinycolor.mix(c, tgt, perc).toHexString();
+ return {
+ base: color,
+ light1: adjustColor(color, 20, fg),
+ light2: adjustColor(color, 45, fg),
+ light3: adjustColor(color, 70, fg),
+ light4: adjustColor(color, 90, fg),
+ light5: adjustColor(color, 95, fg),
+ dark1: adjustColor(color, 10, bg),
+ dark2: adjustColor(color, 20, bg),
+ dark3: adjustColor(color, 40, bg),
+ dark4: adjustColor(color, 60, bg),
+ dark5: adjustColor(color, 80, bg),
+ };
+ }
+
+ private static getColors(
+ systemColors: SystemColors,
+ isDark: boolean,
+ ): DeprecatedThemeColors {
+ /* This method provides a set of color variations based on the system
colors.
+ * Goal is to deprecate usage of these in the future
+ */
+ const sc = systemColors;
+ return {
+ primary: Theme.genDeprecatedColorVariations(sc.colorPrimary, isDark),
+ error: Theme.genDeprecatedColorVariations(sc.colorError, isDark),
+ warning: Theme.genDeprecatedColorVariations(sc.colorWarning, isDark),
+ success: Theme.genDeprecatedColorVariations(sc.colorSuccess, isDark),
+ info: Theme.genDeprecatedColorVariations(sc.colorInfo, isDark),
+ grayscale: Theme.genDeprecatedColorVariations('#666', isDark),
+ };
+ }
+
+ private static augmentSeedWithDefaults(
+ seed: Partial<SupersetTheme>,
+ ): Partial<SupersetTheme> {
+ return {
+ ...Theme.defaultTokens,
+ ...seed,
+ };
+ }
+
+ private static getSystemColors(antdTokens: SharedAntdTokens): SystemColors {
+ return {
+ colorPrimary: antdTokens.colorPrimary,
+ colorError: antdTokens.colorError,
+ colorWarning: antdTokens.colorWarning,
+ colorSuccess: antdTokens.colorSuccess,
+ colorInfo: antdTokens.colorInfo,
+ };
+ }
+
+ private static getSupersetTheme(
+ seed: Partial<SupersetTheme>,
+ isDark = false,
+ ): SupersetTheme {
+ const antdConfig = Theme.getAntdConfig(seed, isDark);
+ const antdTokens = Theme.getFilteredAntdTheme(antdConfig);
+ const systemColors = Theme.getSystemColors(antdTokens);
+
+ const theme: SupersetTheme = {
+ colors: Theme.getColors(systemColors, isDark),
+ ...Theme.defaultTokens,
+ ...antdTokens,
+ };
+ return theme;
+ }
+
+ private static getFilteredAntdTheme(
+ antdConfig: AntdThemeConfig,
+ ): SharedAntdTokens {
+ // This method generates all antd tokens and filters out the ones not
allowed
+ // in Superset
+ const theme = Theme.getAntdTokens(antdConfig);
+ return Object.fromEntries(
+ allowedAntdTokens.map(key => [key, (theme as Record<string, any>)[key]]),
+ ) as SharedAntdTokens;
+ }
+
+ private static getAntdConfig(
+ seed: Partial<SupersetTheme>,
+ isDark: boolean,
+ ): AntdThemeConfig {
+ const algorithm = isDark
+ ? antdThemeImport.darkAlgorithm
+ : antdThemeImport.defaultAlgorithm;
+ return {
+ token: seed,
+ algorithm,
+ };
+ }
+
+ mergeTheme(partialTheme: Partial<LegacySupersetTheme>): void {
+ // const mergedTheme = merge({}, this.theme, partialTheme);
+ // const isDark = tinycolor(mergedTheme.colorBgBase).isDark();
+ // const antdConfig = Theme.getAntdConfig(systemColors, isDark);
+ // this.updateTheme(mergedTheme, antdConfig, isDark);
Review Comment:
Remove this?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
