Feature: update to not use styled-components#228
Conversation
There was a problem hiding this comment.
Pull request overview
This PR migrates the codebase from styled-components to vanilla CSS with CSS custom properties and cascade layers. The migration introduces new CSS files for components (Text, Box, Button) and replaces styled-components with standard React elements using inline styles and CSS classes.
Key Changes
- Removed styled-components dependency and replaced with vanilla CSS files using
@layerfor separation of structure and theme concerns - Introduced legacy compatibility layer (
legacyThemeCompat.ts) with stub functions for backward compatibility during migration - Converted global styles from
createGlobalStyleto a functional component that injects CSS via<style>tags withdangerouslySetInnerHTML
Reviewed changes
Copilot reviewed 12 out of 13 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| src/util/legacyThemeCompat.ts | New compatibility layer providing stub functions for legacy theme exports and inline style conversion utilities |
| src/util/index.ts | Exports the new legacy compatibility functions |
| src/theming/themeContext.tsx | Replaces styled-components' createGlobalStyle with functional component using CSS layers |
| src/particles/text/styles.css | New CSS file defining Text component styles with structure and theme layers |
| src/particles/text/component.tsx | Removes styled-components, uses vanilla <span> with inline styles for dynamic properties |
| src/particles/box/styles.css | New CSS file for Box component with responsive breakpoints and variant classes |
| src/particles/box/component.tsx | Replaces StyledBox with <div> using CSS custom properties for responsive dimensions |
| src/atoms/button/styles.css | New CSS file for Button component with state-based styling |
| src/atoms/button/component.tsx | Removes styled-components, uses dynamic component type (button/a/CoreLink) with inline alignment styles |
| src/app/kibaApp.tsx | Removes StyledMainView in favor of inline styles on standard <div> |
| src/app/globalCss.tsx | Converts from createGlobalStyle to functional component using memoized CSS string injection |
| package.json | Changes build dependency from exact to caret version for pre-release package |
| package-lock.json | Updates lockfile to reflect package.json version change |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const variantClasses = (variant?.split('-') || []).join(' '); | ||
|
|
||
| return ( | ||
| // @ts-ignore: as prop doesn't match type required | ||
| <StyledButton | ||
| // @ts-ignore: dynamic component type | ||
| <Component | ||
| id={props.id} | ||
| className={getClassName(Button.displayName, className, isFullWidth && 'fullWidth', isFullHeight && 'fullHeight', !isEnabled && 'disabled', ...(variant?.split('-') || []))} | ||
| $theme={props.theme} | ||
| $isLoading={props.isLoading || false} | ||
| className={getClassName( | ||
| Button.displayName, | ||
| className, | ||
| 'KibaButton-base', | ||
| isFullWidth && 'fullWidth', | ||
| isFullHeight && 'fullHeight', | ||
| !isEnabled && 'disabled', | ||
| props.isLoading && 'isLoading', | ||
| variantClasses, |
There was a problem hiding this comment.
The variant classes are being joined with spaces and passed as a single class name, but getClassName expects individual arguments to be spread. The line should be ...(variant?.split('-') || []) instead of variantClasses to properly spread the variant class names. Currently, a variant like "primary-large" would be added as a single class "primary large" instead of two separate classes "primary" and "large".
| const shouldBreakOnWords = props.shouldBreakOnWords === true || props.shouldBreakOnWords === undefined; | ||
| const TagComponent = (props.tag || getTextTag(variant)) as React.ElementType; | ||
| const dynamicStyle: React.CSSProperties = { | ||
| ...(lineLimit ? { WebkitLineClamp: lineLimit } : {}), | ||
| ...(shouldBreakOnWords ? { wordBreak: 'break-word' as const } : {}), | ||
| ...(props.shouldBreakAnywhere ? { wordBreak: 'break-all' as const } : {}), | ||
| ...(props.alignment ? { textAlign: props.alignment } : {}), | ||
| }; |
There was a problem hiding this comment.
The alignmentResponsive prop is defined in the component interface but is never used in the implementation. In the old styled-components version, this was handled via fieldToResponsiveCss, but the new implementation only applies props.alignment and ignores props.alignmentResponsive. This means responsive alignment functionality has been lost in the migration.
| const TagComponent = (props.tag || getTextTag(variant)) as React.ElementType; | ||
| const dynamicStyle: React.CSSProperties = { | ||
| ...(lineLimit ? { WebkitLineClamp: lineLimit } : {}), | ||
| ...(shouldBreakOnWords ? { wordBreak: 'break-word' as const } : {}), | ||
| ...(props.shouldBreakAnywhere ? { wordBreak: 'break-all' as const } : {}), |
There was a problem hiding this comment.
Conflicting wordBreak assignments can result in the wrong value being applied. If both shouldBreakOnWords and shouldBreakAnywhere are true, the spread operator will overwrite 'break-word' with 'break-all'. Consider using a single conditional to determine the correct wordBreak value, e.g., wordBreak: props.shouldBreakAnywhere ? 'break-all' : (shouldBreakOnWords ? 'break-word' : undefined).
| const TagComponent = (props.tag || getTextTag(variant)) as React.ElementType; | |
| const dynamicStyle: React.CSSProperties = { | |
| ...(lineLimit ? { WebkitLineClamp: lineLimit } : {}), | |
| ...(shouldBreakOnWords ? { wordBreak: 'break-word' as const } : {}), | |
| ...(props.shouldBreakAnywhere ? { wordBreak: 'break-all' as const } : {}), | |
| const wordBreak = props.shouldBreakAnywhere | |
| ? 'break-all' | |
| : (shouldBreakOnWords ? 'break-word' : undefined); | |
| const TagComponent = (props.tag || getTextTag(variant)) as React.ElementType; | |
| const dynamicStyle: React.CSSProperties = { | |
| ...(lineLimit ? { WebkitLineClamp: lineLimit } : {}), | |
| ...(wordBreak ? { wordBreak } : {}), |
| const combinedStyles: React.CSSProperties = { | ||
| ...themeStyles, | ||
| // @ts-expect-error CSS custom properties are valid but not in CSSProperties type | ||
| '--kiba-box-display': blockType, | ||
| '--kiba-box-width': width, | ||
| '--kiba-box-width-sm': props.widthResponsive?.small, | ||
| '--kiba-box-width-md': props.widthResponsive?.medium, | ||
| '--kiba-box-width-lg': props.widthResponsive?.large, | ||
| '--kiba-box-width-xl': props.widthResponsive?.extraLarge, | ||
| '--kiba-box-height': height, | ||
| '--kiba-box-height-sm': props.heightResponsive?.small, | ||
| '--kiba-box-height-md': props.heightResponsive?.medium, | ||
| '--kiba-box-height-lg': props.heightResponsive?.large, | ||
| '--kiba-box-height-xl': props.heightResponsive?.extraLarge, | ||
| '--kiba-box-max-width': props.maxWidth, | ||
| '--kiba-box-max-width-sm': props.maxWidthResponsive?.small, | ||
| '--kiba-box-max-width-md': props.maxWidthResponsive?.medium, | ||
| '--kiba-box-max-width-lg': props.maxWidthResponsive?.large, | ||
| '--kiba-box-max-width-xl': props.maxWidthResponsive?.extraLarge, | ||
| '--kiba-box-max-height': props.maxHeight, | ||
| '--kiba-box-max-height-sm': props.maxHeightResponsive?.small, | ||
| '--kiba-box-max-height-md': props.maxHeightResponsive?.medium, | ||
| '--kiba-box-max-height-lg': props.maxHeightResponsive?.large, | ||
| '--kiba-box-max-height-xl': props.maxHeightResponsive?.extraLarge, | ||
| '--kiba-box-min-width': props.minWidth, | ||
| '--kiba-box-min-width-sm': props.minWidthResponsive?.small, | ||
| '--kiba-box-min-width-md': props.minWidthResponsive?.medium, | ||
| '--kiba-box-min-width-lg': props.minWidthResponsive?.large, | ||
| '--kiba-box-min-width-xl': props.minWidthResponsive?.extraLarge, | ||
| '--kiba-box-min-height': props.minHeight, | ||
| '--kiba-box-min-height-sm': props.minHeightResponsive?.small, | ||
| '--kiba-box-min-height-md': props.minHeightResponsive?.medium, | ||
| '--kiba-box-min-height-lg': props.minHeightResponsive?.large, | ||
| '--kiba-box-min-height-xl': props.minHeightResponsive?.extraLarge, | ||
| '--kiba-box-z-index': props.zIndex !== undefined ? String(props.zIndex) : undefined, | ||
| '--kiba-box-position': props.position, | ||
| }; |
There was a problem hiding this comment.
Setting CSS custom properties to undefined may result in the string 'undefined' being assigned as the CSS value instead of being omitted. This will break the CSS fallback chain. Consider filtering out undefined values before constructing the styles object, or explicitly set them to null or empty string when they should be omitted.
| className={getClassName( | ||
| Button.displayName, | ||
| className, | ||
| 'KibaButton-base', |
There was a problem hiding this comment.
The class name 'KibaButton-base' is added but does not appear to be defined in the CSS file. This will have no effect and should either be removed or the corresponding CSS should be added to styles.css.
| style={{ | ||
| // @ts-ignore | ||
| alignItems: childAlignment, | ||
| // @ts-ignore | ||
| justifyContent: contentAlignment, | ||
| }} |
There was a problem hiding this comment.
The Alignment enum values are being used directly for CSS properties, but they need to be converted first. The old implementation used getChildAlignmentCss and getContentAlignmentCss functions which convert enum values like Alignment.Start to valid CSS values like flex-start.
The code should use getFlexItemAlignment(childAlignment) for alignItems and getFlexContentAlignment(contentAlignment) for justifyContent instead of using the raw enum values.
| }, | ||
| "devDependencies": { | ||
| "@kibalabs/build": "0.13.3-next.10", | ||
| "@kibalabs/build": "^0.13.3-next.10", |
There was a problem hiding this comment.
The version specifier was changed from an exact version 0.13.3-next.10 to a caret range ^0.13.3-next.10. This is inconsistent with how pre-release versions should be handled. The caret (^) operator treats pre-release versions specially and may not behave as expected. For pre-release versions, it's better to use exact versions or tilde (~) to avoid unexpected updates.
| "@kibalabs/build": "^0.13.3-next.10", | |
| "@kibalabs/build": "0.13.3-next.10", |
Description
Screenshots:
Checklist: