Feature/config provider #614

interim17 · 2024-11-20T23:50:27Z

Time estimate or Size

small

Problem

Advances #601 Closes #603 #604

With antd bumped to v5, ant-vars.less is deprecated and replaced by ConfigProvider.

This PR is for making the switch and capturing the styles that were being handled by less.

It is not a comprehensive fix of styling. Migrating lines from ant-vars.less is not sufficient to make things look right, and completing that process is out of scope.

Solution

delete ant.var.less
wrap App with ConfigProvider and define SimulariumTheme with global and component tokens to mostly replace the styles applied in ant-vars.less
TS base color variables updated to match CSS vars, semantic variable names grouped according to light and dark theme to reflect that same distinction in current CSS although there is only one simulariumTheme used in ConfigProvider

We can decide about having different antd theme algorithms going forward.

I think reasonable approval criteria here are:

the ConfigProvider styles is being applied
nothing seems garishly out of place in terms of component tokens or file organization.

The point here is to get rid of ant-vars.less, migrate the gist of what styles it was still handling, and establish the ConfigProvider

…rk with new versions

…um-website into feature/config-provider

… theme colors

meganrm · 2024-11-25T21:10:27Z

src/index.tsx

-                    <RouterSwitch />
-                </BrowserRouter>
-            </Layout>
+            <ConfigProvider theme={simulariumTheme} wave={{ disabled: true }}>


what is wave?

wave controls some animations that are hard to turn off individually

meganrm · 2024-11-25T21:17:40Z

src/styles/theme/themeConfig.ts

+    inputPlaceholderColor: WARM_GRAY,
+    inputBorderColor: WARM_GRAY,
+    inputBgColor: OFF_WHITE,
+    inputHoverColor: BABY_PURPLE,


Overall this is nice, but some of the repetition here is more detailed than I think is necessary. For instance, I would have a single light AppBgColor, and then assign it below for both colorBgContainer and inputBgColor because you're always going to want those two to change together, instead of having a 1:1 of the token names in here. Similarly I would have a single pageBgColor: WHITE_SIX and use it for the base and the layout. I'm also surprised you have to define the inputHoverColor separately, doesn't it already get the primaryColor?

Ok I will do some consolidating!

meganrm · 2024-11-25T21:18:12Z

src/styles/theme/themeConfig.ts

+    menuDarkSelectedColor: PURE_WHITE,
+
+    // Button
+    buttonPrimaryColor: BABY_PURPLE,


again, doesn't this already get set by setting colorPrimary?

meganrm · 2024-11-25T21:19:29Z

src/styles/theme/themeConfig.ts

+    tabsInkBarColor: DARK_FOUR,
+    tabsItemHoverColor: DARK_FOUR,
+    tabsItemActiveColor: DARK_FOUR,
+    tabsItemSelectedColor: DARK_FOUR,


combine these into one variable tabsInteractionColor or something

meganrm · 2024-11-25T21:25:11Z

src/styles/theme/themeConfig.ts

+    sliderRailHoverBg: WARM_GRAY,
+    sliderTrackBg: WHITE_THREE,
+    sliderHandleColor: WHITE_THREE,
+    sliderDotBorderColor: WHITE_THREE,


also combine all these into one sliderColor

meganrm

looks good, I think you should reduce your variables in your mappings though. The goal is when we change a color, we can change the fewest lines of code possible. I think it's a safe assumption that if things are currently the same color as each other we'll want to keep them the same color. So your theme definitions can have a lot fewer variables than the antd theme, and just reuse them (which you're doing in some places already)

meganrm · 2024-12-03T18:36:55Z

src/styles/theme/themeConfig.ts

+    // Layout
+    layoutSiderBg: DARK,
+    layoutTriggerBg: DARK,
+    layoutTriggerColor: BABY_PURPLE,


Same thing as before: you positive you need to set this separately when you already have primary color set to baby purple?

meganrm · 2024-12-03T18:37:13Z

src/styles/theme/themeConfig.ts

+const APP_BG_COLOR = OFF_WHITE;
+const TABS_INTERACTION_COLOR = DARK_FOUR;
+const SLIDER_COLOR = WHITE_THREE;
+const CHECKBOX_BG_COLOR = WHITE_SIX;


Nit: just call this LAYOUT_BG and use it for colorBgBase and colorBgLayout below. or don't have both of those in your light theme and just set them in the actual theme.

meganrm · 2024-12-03T18:41:53Z

src/styles/theme/themeConfig.ts

+const darkTheme = {
+    // Layout
+    layoutSiderBg: DARK,
+    layoutTriggerBg: DARK,


nit: combine these into one variable

meganrm · 2024-12-03T18:44:12Z

src/styles/theme/themeConfig.ts

+            headerPadding: 0,
+            siderBg: darkTheme.layoutSiderBg,
+            triggerBg: darkTheme.layoutTriggerBg,
+            triggerColor: darkTheme.layoutTriggerColor,


if you do need to define this, I would just use colorPrimary instead of making a new variable here, which makes it less clear that this just is the primary color

meganrm · 2024-12-03T18:46:00Z

src/styles/theme/themeConfig.ts

+
+    // Dropdown
+    dropdownBgColor: CHARCOAL_GRAY,
+    dropdownTextColor: TRANSPARENT_WHITE,


nit: call it FADED_WHITE because transparent sounds like it's going to be invisible

meganrm

looks good, I left a few more suggestions but nothing blocking

frasercl

Looks good!

frasercl · 2024-12-03T23:45:34Z

src/styles/theme/themeConfig.ts

+const CHECKBOX_BG_COLOR = WHITE_SIX;
+
+// Light Theme Tokens (Global defaults, Modal, Radio)
+const lightTheme = {


I've gotten confused working with this repo in the past by assuming that "light theme" and "dark theme" were different modes for the whole UI that could be switched between dynamically, rather than design concepts that statically apply to different UI elements (light theme for landing page, dark theme for viewer controls, etc.). Consider adding a few extra words to this comment to help out confused devs like my past self.

Also: even given that these "themes" apply to different UI elements, I'm a bit surprised that more effort isn't taken to unify the shapes of these objects, but that's out of scope for this PR anyways.

Hate to say it but I think I have found an even better shiny new way of organizing these variables, so this may code might not last that long. This uses styled-components so is pending some buy-in from reviewer and Megan but I like it and its pointed at this branch. It gets rid of the light and dark theme naming you see here.
#616

I agree that the light/dark theme is confusing, and I am also hoping to get rid of the light-theme.css file in favor of some CSS-in-JS and actually using themes for theming when things are called themes.

interim17 added 5 commits November 4, 2024 12:34

bump antd and typescript, and update build tools and components to wo…

ee6d0d7

…rk with new versions

consolidate ts.config

334431a

Merge branch 'feature/ant5' of https://github.com/simularium/simulari…

1c629d5

…um-website into feature/config-provider

replace ant-vars.less with ConfigProvider

ad19491

semantic color variables names for config provider

2a6b563

interim17 marked this pull request as ready for review November 21, 2024 00:10

interim17 requested a review from a team as a code owner November 21, 2024 00:10

interim17 requested review from meganrm and frasercl and removed request for a team November 21, 2024 00:10

interim17 marked this pull request as draft November 21, 2024 00:43

align css var colors and theme color variables, divide light and dark…

f24d313

… theme colors

interim17 marked this pull request as ready for review November 21, 2024 07:04

meganrm reviewed Nov 25, 2024

View reviewed changes

meganrm requested changes Nov 25, 2024

View reviewed changes

consolidate theme variables

58c7ea1

interim17 requested a review from meganrm November 28, 2024 00:07

meganrm reviewed Dec 3, 2024

View reviewed changes

meganrm approved these changes Dec 3, 2024

View reviewed changes

frasercl approved these changes Dec 3, 2024

View reviewed changes

further consolidate theme variables

f9445c5

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/config provider #614

Feature/config provider #614

interim17 commented Nov 20, 2024 •

edited

Loading

meganrm Nov 25, 2024

interim17 Nov 27, 2024

meganrm Nov 25, 2024

interim17 Nov 27, 2024

meganrm Nov 25, 2024

meganrm Nov 25, 2024

meganrm Nov 25, 2024

meganrm left a comment

meganrm Dec 3, 2024

meganrm Dec 3, 2024

meganrm Dec 3, 2024

meganrm Dec 3, 2024

meganrm Dec 3, 2024

meganrm left a comment

frasercl left a comment

frasercl Dec 3, 2024

interim17 Dec 4, 2024

Feature/config provider #614

Are you sure you want to change the base?

Feature/config provider #614

Conversation

interim17 commented Nov 20, 2024 • edited Loading

Time estimate or Size

Problem

Solution

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

Choose a reason for hiding this comment

meganrm 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

meganrm left a comment

Choose a reason for hiding this comment

frasercl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

interim17 commented Nov 20, 2024 •

edited

Loading