refactor(theme): update config for `extendBaseTheme` #9681

TylerAPfledderer · 2023-03-09T04:50:04Z

Description

Applies Chakra's extendBaseTheme function for the custom config and updates the config to reflect this change.

The purpose of this function versus the previously used extendTheme is to not immediately include all of the default component themes.
Default themes are imported as needed to ensure the current UI is retained
A small reusable function is created to allow for recursive merging (deep nested merging) of default and custom styling to prevent any unnecessary overrides.

Certainly open for trimming any of this, either now or in a future PR.

Related Issue

Closes #9679

TylerAPfledderer · 2023-03-09T04:57:17Z

src/@chakra-ui/gatsby-plugin/components/components.utils.ts

+const {
+  Accordion: accordionDefaultTheme,
+  Avatar: avatarDefaultTheme,
+  Badge: badgeDefaultTheme,
+  Breadcrumb: breadcrumbDefaultTheme,
+  Button: buttonDefaultTheme,
+  Checkbox: checkboxDefaultTheme,
+  CloseButton: closeButtonDefaultTheme,
+  Code: codeDefaultTheme,
+  Divider: dividerDefaultTheme,
+  Drawer: drawerDefaultTheme,
+  Form: formDefaultTheme,
+  FormLabel: formLabelDefaultTheme,
+  Heading: headingDefaultTheme,
+  Input: inputDefaultTheme,
+  Link: linkDefaultTheme,
+  List: listDefaultTheme,
+  Menu: menuDefaultTheme,
+  Modal: modalDefaultTheme,
+  Select: selectDefaultTheme,
+  Spinner: spinnerDefaultTheme,
+  Switch: switchDefaultTheme,
+  Table: tableDefaultTheme,
+  Tabs: tabsDefaultTheme,
+  Tag: tagDefaultTheme,
+} = theme.components


Import aliases used for consistency, as some of these defaults are exported to custom component configs.

The rest are imported to retain the current UI with all Chakra components used that have themes, and includes themes for components that may be used later as the UI migration continues and other refactoring needs.

It is possible that this list could be trimmed down with any discovery of themes that are not actually needed.

TylerAPfledderer · 2023-03-09T05:26:25Z

src/@chakra-ui/gatsby-plugin/components/components.utils.ts

+export function defineMergeStyles(
+  defaultTheming?: SystemStyleObject,
+  ...styleObjs: SystemStyleObject[] | undefined[]
+) {
+  return merge(defaultTheming, ...styleObjs)
+}


This function was created for re-usability to handle merging of default styles with custom stylings. It uses the lodash merge function to handle recursive merging to prevent unexpected overrides.

Though this works as intended, I believe this could be improved, as there are multiple circumstances that this would be used in.

Ex. 1: Simple merge

// From custom Link config baseStyle: defineMergeStyles(linkDefaultTheme.baseStyle, { color: "primary", textDecoration: "underline", _focus: { boxShadow: "none", }, _focusVisible: { boxShadow: "none", outline: "auto", }, }, })

Ex. 2: Style function props needed to be passed to a default theme that uses some of those props (type error throws otherwise)

// From custom Button config const variantOutline = defineStyle((props) => defineMergeStyles(defaultVariants?.outline(props), commonOutline) )

Ex. 3: There may be a circumstance where a default baseStyle would be combined with a default variant because the component may not use other variants in the project, thereby compacting the custom config.

const baseStyle = defineMergeStyles( defaultBaseStyle, defaultVariant.outline, { // Custom styles } )

NOTE: There is a mergeThemeOverride that is available from Chakra that essentially does the same thing, but it doesn't have any type safety.

Awesome. Would love to have this documented somewhere...I feel bad to lose this doc in this PR.

======
On the other hand, merge will return any, taking away the whole purpose of this function I believe. Shouldn't we do something like:
🤔

Suggested change

export function defineMergeStyles(

defaultTheming?: SystemStyleObject,

...styleObjs: SystemStyleObject[] | undefined[]

) {

return merge(defaultTheming, ...styleObjs)

}

export function defineMergeStyles(

defaultTheming?: SystemStyleObject,

...styleObjs: SystemStyleObject[] | undefined[]

) {

return merge(defaultTheming, ...styleObjs) as SystemStyleObject

}

or

Suggested change

export function defineMergeStyles(

defaultTheming?: SystemStyleObject,

...styleObjs: SystemStyleObject[] | undefined[]

) {

return merge(defaultTheming, ...styleObjs)

}

export function defineMergeStyles<T extends SystemStyleObject>(

defaultTheming?: T,

...styleObjs: T[] | undefined[]

) {

return merge(defaultTheming, ...styleObjs) as T

}

@pettinarip

Awesome. Would love to have this documented somewhere...I feel bad to lose this doc in this PR.

I can put this up in the PR description as Additional Information 😃

With your second suggestion, this approach will return a type equivalent only to the first object, which would be the default theme, and therefore lock down what the second object could have in its properties.

With the first suggestion, this will throw a type error in configs where the function is used in a defineMultiStyleConfig property.

For example, the sizes property in the Button theme config. The error thrown here:

Type 'SystemStyleObject' is not assignable to type 'Dict<SystemStyleInterpolation> | undefined'

Dict is currently a private type in Chakra, but it's simply

type Dict<T = any> = { [ x: string ]: T }

We could apply this assertion on the return, written as Record<string, SystemStyleObject> which will work everywhere.

export function defineMergeStyles( defaultTheming?: SystemStyleObject | unknown, ...styleObjs: SystemStyleObject[] | unknown[] ) { return merge(defaultTheming, ...styleObjs) as Record<string, SystemStyleObject> }

Note that for both params, I am also using unknown for any object that is inferring as an object or undefined. (Type error throws otherwise because of either an object mismatch for defaultTheming or of the array type for styleObjs, if that makes sense)

gatsby-cloud · 2023-03-09T05:34:05Z

✅ ethereum-org-website-dev deploy preview ready

Deploy preview
Build logs · 49m build time

pettinarip

Amazing @TylerAPfledderer!

A few minor observations but this looks really nice. GJ

pettinarip · 2023-03-10T17:24:10Z

src/@chakra-ui/gatsby-plugin/components/Badge.ts


-const variantSecondary: SystemStyleObject = {
+const { baseStyle: defaultBaseStyle, sizes: defaultSizes } = badgeDefaultTheme


Suggested change

const { baseStyle: defaultBaseStyle, sizes: defaultSizes } = badgeDefaultTheme

const { baseStyle: defaultBaseStyle } = badgeDefaultTheme

pettinarip · 2023-03-10T17:28:50Z

src/@chakra-ui/gatsby-plugin/components/Button.ts

+
+export const Button = defineStyleConfig({
+  baseStyle,
+  sizes: defineMergeStyles(defaultSizes, {
    md: {
      h: 42,


This was my mistake, this should be

Suggested change

h: 42,

h: "42px",

pettinarip · 2023-03-10T18:28:40Z

src/@chakra-ui/gatsby-plugin/components/Checkbox.ts

+const { baseStyle: defaultBaseStyle } = checkboxDefaultTheme
+
+const { definePartsStyle, defineMultiStyleConfig } =
+  createMultiStyleConfigHelpers(checkboxAnatomy.keys)


Interesting, nice.

pettinarip · 2023-03-10T18:32:48Z

src/@chakra-ui/gatsby-plugin/components/Checkbox.ts

-        mt: "0.25rem",
-      },
-    },
+    alignTop: variantAlignTop,


Suggested change

alignTop: variantAlignTop,

// TODO: remove this variant

alignTop: variantAlignTop,

pettinarip · 2023-03-10T18:36:40Z

src/@chakra-ui/gatsby-plugin/components/Modal.ts

@@ -1,46 +1,69 @@
-import { StyleFunctionProps } from "@chakra-ui/react"
-import type { ComponentStyleConfig } from "@chakra-ui/theme"
+import { createMultiStyleConfigHelpers, defineStyle } from "@chakra-ui/react"


Suggested change

import { createMultiStyleConfigHelpers, defineStyle } from "@chakra-ui/react"

import { createMultiStyleConfigHelpers } from "@chakra-ui/react"

pettinarip · 2023-03-10T18:49:11Z

src/@chakra-ui/gatsby-plugin/components/components.utils.ts

+export function defineMergeStyles(
+  defaultTheming?: SystemStyleObject,
+  ...styleObjs: SystemStyleObject[] | undefined[]
+) {
+  return merge(defaultTheming, ...styleObjs)
+}


Awesome. Would love to have this documented somewhere...I feel bad to lose this doc in this PR.

======
On the other hand, merge will return any, taking away the whole purpose of this function I believe. Shouldn't we do something like:
🤔

Suggested change

export function defineMergeStyles(

defaultTheming?: SystemStyleObject,

...styleObjs: SystemStyleObject[] | undefined[]

) {

return merge(defaultTheming, ...styleObjs)

}

export function defineMergeStyles(

defaultTheming?: SystemStyleObject,

...styleObjs: SystemStyleObject[] | undefined[]

) {

return merge(defaultTheming, ...styleObjs) as SystemStyleObject

}

or

Suggested change

export function defineMergeStyles(

defaultTheming?: SystemStyleObject,

...styleObjs: SystemStyleObject[] | undefined[]

) {

return merge(defaultTheming, ...styleObjs)

}

export function defineMergeStyles<T extends SystemStyleObject>(

defaultTheming?: T,

...styleObjs: T[] | undefined[]

) {

return merge(defaultTheming, ...styleObjs) as T

}

pettinarip · 2023-03-10T18:58:52Z

src/@chakra-ui/gatsby-plugin/components/Tag.ts

@@ -37,4 +49,8 @@ export const Tag: ComponentStyleConfig = {
    },
    custom: {}, // empty variant


Suggested change

custom: {}, // empty variant

// TODO: remove this variant, as it doesn't exist on the DS

custom: {}, // empty variant

pettinarip · 2023-03-27T15:42:01Z

@TylerAPfledderer

Not a big deal but I was analyzing the final bundles and noticed that the size had an unexpected result. They have basically the same size.

dev build (extendTheme):
@chakra-ui bundle size 277.01 kb

This PR build (extendBaseTheme):
@chakra-ui bundle size 286.39 kb

I was expecting a smaller size overall but we got an even slightly heavier one. Do you know what could be the reason? maybe is that we are not excluding a lot of Chakra components after all? 🤔

But again, not a big deal, regardless of the bundle size, I think this is a great improvement to reduce the number of theme styles we have.

TylerAPfledderer · 2023-03-27T16:24:34Z

@pettinarip I think it would still be hard to say at this point. It makes sense if the lack of excluded default component themes being imported, but I am expecting some trimming from the imports after starting work on PRs for the form elements, tags, tables, and avatars with their theming.

I would still find the results not telling when styled-components and the direct inclusion of emotion still a part of the package. Maybe it doesn't make a difference here!

I think what would be good for the Chakra team is to keep record of at least three bundle size points:

Bundle sizes before adding Chakra to the project
Bundle Size before the use of extendBaseTheme
Bundle Size after the removal of all the old theming imports from Styled-components and emotion

I'm not sure if this will make a difference, but what if indeed I allowed the rendering of ChakraBaseProvider what that would do to the bundle along with extendBaseTheme?

I've argued that this provider is a redundancy but would also be good to notate that result.

I wanted to save these bundle size notes for the case study so it's worth bringing this up 😁

pettinarip · 2023-03-27T22:36:03Z

@TylerAPfledderer cool. Then, are you ok if I merge this PR? lgtm.

After that, you can play around with ChakraBaseProvider and see if that has any real impact. LMK what you think

TylerAPfledderer · 2023-03-27T22:38:46Z

@pettinarip yes, go ahead and merge! 🚀

refactor(theme): update config for extendBaseTheme

9f59af2

TylerAPfledderer requested review from samajammin, wackerow, corwintines, pettinarip, minimalsm and jmcook1186 as code owners March 9, 2023 04:50

github-actions bot added the review needed 👀 label Mar 9, 2023

TylerAPfledderer commented Mar 9, 2023

View reviewed changes

refactor(theme/utils): update typing for defineMergeStyles

2738c6c

TylerAPfledderer commented Mar 9, 2023

View reviewed changes

pettinarip approved these changes Mar 10, 2023

View reviewed changes

pettinarip and others added 7 commits March 10, 2023 19:12

Merge branch 'dev' into pr/TylerAPfledderer/9681

1573a8c

chore(theme/Badge): remove unused sizes destructure

c55b87b

fix(theme/Button): fix syntax for md size height

a2acbd7

chore(theme/Checkbox): add comment to remove variant

3ba1974

chore(theme/Modal): remove unused import

ddaf882

chore(theme/Tag): add comment to remove custom variant

62f660d

Merge branch 'dev' into pr/TylerAPfledderer/9681-1

19b3d03

pettinarip requested a review from nhsz as a code owner March 24, 2023 20:28

pettinarip merged commit f5d7670 into ethereum:dev Mar 27, 2023

corwintines mentioned this pull request Mar 29, 2023

Deploy v7.7.2 #9831

Merged

TylerAPfledderer deleted the feat/extend-base-theme branch March 29, 2023 21:55

TylerAPfledderer mentioned this pull request Apr 8, 2023

feat(Avatar): create component, theme, and stories #9868

Merged

oBusk mentioned this pull request Apr 14, 2023

The same bundle size with ChakraBaseProvider and ChakraProvider chakra-ui/chakra-ui#7383

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(theme): update config for `extendBaseTheme` #9681

refactor(theme): update config for `extendBaseTheme` #9681

TylerAPfledderer commented Mar 9, 2023 •

edited

Loading

TylerAPfledderer Mar 9, 2023

TylerAPfledderer Mar 9, 2023 •

edited

Loading

pettinarip Mar 10, 2023

TylerAPfledderer Mar 13, 2023 •

edited

Loading

gatsby-cloud bot commented Mar 9, 2023 •

edited

Loading

pettinarip left a comment

pettinarip Mar 10, 2023

pettinarip Mar 10, 2023

pettinarip Mar 10, 2023

pettinarip Mar 10, 2023

pettinarip Mar 10, 2023

pettinarip Mar 10, 2023

pettinarip Mar 10, 2023

pettinarip commented Mar 27, 2023 •

edited

Loading

TylerAPfledderer commented Mar 27, 2023 •

edited

Loading

pettinarip commented Mar 27, 2023

TylerAPfledderer commented Mar 27, 2023


		const variantSecondary: SystemStyleObject = {
		const { baseStyle: defaultBaseStyle, sizes: defaultSizes } = badgeDefaultTheme

	const { baseStyle: defaultBaseStyle, sizes: defaultSizes } = badgeDefaultTheme
	const { baseStyle: defaultBaseStyle } = badgeDefaultTheme

	alignTop: variantAlignTop,
	// TODO: remove this variant
	alignTop: variantAlignTop,

	import { createMultiStyleConfigHelpers, defineStyle } from "@chakra-ui/react"
	import { createMultiStyleConfigHelpers } from "@chakra-ui/react"

		@@ -37,4 +49,8 @@ export const Tag: ComponentStyleConfig = {
		},
		custom: {}, // empty variant

	custom: {}, // empty variant
	// TODO: remove this variant, as it doesn't exist on the DS
	custom: {}, // empty variant

refactor(theme): update config for extendBaseTheme #9681

refactor(theme): update config for extendBaseTheme #9681

Conversation

TylerAPfledderer commented Mar 9, 2023 • edited Loading

Description

Related Issue

Choose a reason for hiding this comment

TylerAPfledderer Mar 9, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TylerAPfledderer Mar 13, 2023 • edited Loading

Choose a reason for hiding this comment

gatsby-cloud bot commented Mar 9, 2023 • edited Loading

✅ ethereum-org-website-dev deploy preview ready

pettinarip 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pettinarip commented Mar 27, 2023 • edited Loading

TylerAPfledderer commented Mar 27, 2023 • edited Loading

pettinarip commented Mar 27, 2023

TylerAPfledderer commented Mar 27, 2023

refactor(theme): update config for `extendBaseTheme` #9681

refactor(theme): update config for `extendBaseTheme` #9681

TylerAPfledderer commented Mar 9, 2023 •

edited

Loading

TylerAPfledderer Mar 9, 2023 •

edited

Loading

TylerAPfledderer Mar 13, 2023 •

edited

Loading

gatsby-cloud bot commented Mar 9, 2023 •

edited

Loading

pettinarip commented Mar 27, 2023 •

edited

Loading

TylerAPfledderer commented Mar 27, 2023 •

edited

Loading