feat(Accordion): moved to emotion styles #1470
Conversation
WalkthroughThe pull request introduces a comprehensive refactoring of the Accordion component's styling approach, transitioning from SCSS modules to Emotion CSS. This involves adding Changes
Suggested labels
Suggested reviewers
Possibly related PRs
Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
marcinsawicki
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (10)
packages/react-components/src/components/Accordion/styles.ts (6)
11-41: Optimize transition performance.Instead of transitioning 'all' properties, specify only the properties that need to change for better performance.
- transition: all var(${TransitionDurationToken.Moderate1}); + transition: border-color var(${TransitionDurationToken.Moderate1}), + box-shadow var(${TransitionDurationToken.Moderate1}), + background-color var(${TransitionDurationToken.Moderate1});
43-61: Add type safety for kind parameter.Consider using a union type for the kind parameter to prevent potential errors.
type AccordionKind = 'warning' | 'error'; export const kind = (kind?: AccordionKind) => css`
73-87: Enhance keyboard accessibility.Add focus styles to improve keyboard navigation experience.
&:hover { cursor: pointer; } + &:focus-visible { + outline: none; + box-shadow: var(${ShadowToken.Focus}); + }
89-107: Add RTL support for chevron positioning.Consider using logical properties for better RTL support.
- right: 20px; + inset-inline-end: 20px;
115-133: Extract common padding logic.Consider creating a shared function for isPromo padding to avoid duplication.
const getPromoPadding = (isPromo: boolean) => isPromo ? SpacingToken.Spacing6 : SpacingToken.Spacing5;
135-146: Remove padding logic duplication.The padding logic is duplicated. Use the suggested shared function from the previous comment.
export const footer = (isPromo?: boolean) => css` border-top: 1px solid var(${DesignToken.BorderBasicSecondary}); - padding: var(${SpacingToken.Spacing5}); - - ${isPromo - ? ` - padding: var(${SpacingToken.Spacing6}); - ` - : ` - padding: var(${SpacingToken.Spacing5}); - `} + padding: var(${getPromoPadding(isPromo)}); `;packages/react-components/src/components/Accordion/components/AccordionMultilineElement/styles.ts (1)
11-14: Consider adding type safety for style exportsConsider using const assertion to prevent accidental style mutations:
-export const baseStyles = css` +export const baseStyles = css` as const; -export const inner = css` +export const inner = css` as const;packages/react-components/src/components/Accordion/components/AccordionAnimatedLabel/styles.ts (2)
5-19: Consider memoizing style functionsThe style function is recreated on every render. Consider memoizing it for better performance.
+import { useMemo } from 'react'; -export const baseStyles = (containerHeight?: number) => css` +export const useBaseStyles = (containerHeight?: number) => useMemo( + () => css` display: flex; position: relative; align-items: center; transition: all var(${TransitionDurationToken.Fast2}) ease-in-out; min-height: 24px; ${containerHeight ? ` height: ${containerHeight}px; ` : ` height: auto; `} -`; +`, [containerHeight]);
21-33: Same memoization recommendation applies hereSimilar performance optimization can be applied to the element styles.
-export const element = (isVisible: boolean) => css` +export const useElement = (isVisible: boolean) => useMemo( + () => css` position: absolute; transition: all var(${TransitionDurationToken.Fast2}) ease-in-out; max-width: 100%; ${isVisible ? ` opacity: 1; ` : ` opacity: 0; `} -`; +`, [isVisible]);packages/react-components/src/components/Accordion/components/AccordionMultilineElement/AccordionMultilineElement.tsx (1)
24-24: Consider using cx for className compositionFor better type safety and className composition, consider using emotion's cx utility.
+import { cx } from '@emotion/css'; -className={styles.baseStyles} +className={cx(styles.baseStyles)} -className={styles.inner} +className={cx(styles.inner)}Also applies to: 29-29
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (13)
packages/react-components/package.json(2 hunks)packages/react-components/src/components/Accordion/Accordion.module.scss(0 hunks)packages/react-components/src/components/Accordion/Accordion.tsx(8 hunks)packages/react-components/src/components/Accordion/components/AccordionAnimatedLabel.module.scss(0 hunks)packages/react-components/src/components/Accordion/components/AccordionAnimatedLabel/AccordionAnimatedLabel.spec.tsx(1 hunks)packages/react-components/src/components/Accordion/components/AccordionAnimatedLabel/AccordionAnimatedLabel.tsx(2 hunks)packages/react-components/src/components/Accordion/components/AccordionAnimatedLabel/styles.ts(1 hunks)packages/react-components/src/components/Accordion/components/AccordionMultilineElement.module.scss(0 hunks)packages/react-components/src/components/Accordion/components/AccordionMultilineElement/AccordionMultilineElement.tsx(2 hunks)packages/react-components/src/components/Accordion/components/AccordionMultilineElement/styles.ts(1 hunks)packages/react-components/src/components/Accordion/components/index.ts(1 hunks)packages/react-components/src/components/Accordion/helpers.tsx(1 hunks)packages/react-components/src/components/Accordion/styles.ts(1 hunks)
💤 Files with no reviewable changes (3)
- packages/react-components/src/components/Accordion/components/AccordionAnimatedLabel.module.scss
- packages/react-components/src/components/Accordion/components/AccordionMultilineElement.module.scss
- packages/react-components/src/components/Accordion/Accordion.module.scss
✅ Files skipped from review due to trivial changes (2)
- packages/react-components/src/components/Accordion/components/AccordionAnimatedLabel/AccordionAnimatedLabel.spec.tsx
- packages/react-components/src/components/Accordion/helpers.tsx
🔇 Additional comments (11)
packages/react-components/src/components/Accordion/styles.ts (3)
1-10: LGTM! Well-organized imports.Clean import structure using named imports for design tokens.
63-71: LGTM! Clean open state implementation.Consistent use of design tokens for styling.
109-113: LGTM! Clean content styles.Minimal and effective implementation.
packages/react-components/src/components/Accordion/components/index.ts (1)
1-2: Clean and clear exports!The barrel exports follow best practices for component organization.
packages/react-components/src/components/Accordion/components/AccordionMultilineElement/styles.ts (1)
5-9: Style definition looks good!Clean transition from SCSS to emotion styles.
packages/react-components/src/components/Accordion/components/AccordionAnimatedLabel/AccordionAnimatedLabel.tsx (3)
3-6: Import paths look good!The updated paths correctly reflect the new directory structure.
38-38: Good use of functional styles!The dynamic height calculation is properly passed to the style function.
40-47: Clean implementation of conditional styles!The visibility states are handled consistently for both open and closed elements.
packages/react-components/src/components/Accordion/Accordion.tsx (2)
3-3: Excellent emotion migration!The transition from SCSS modules to emotion is clean and consistent throughout the component.
Also applies to: 13-13, 56-56, 66-66, 85-85, 94-94, 103-103, 112-112
126-126: Well-structured component variants!The base styles are properly differentiated between regular and promo variants.
Also applies to: 132-132
packages/react-components/package.json (1)
37-37: Verify @emotion/css version compatibility.Please ensure version 11.13.0 is compatible with the rest of the dependencies.
Run this script to check for potential version conflicts:
Also applies to: 78-78
✅ Verification successful
@emotion/css version ^11.13.0 is compatible
The version is recent, safely allows minor updates, and has no conflicts with other dependencies.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for potential version conflicts with @emotion/css # Get the latest version from npm echo "Latest @emotion/css version:" npm view @emotion/css version # Check for any dependencies that require a specific @emotion/css version npm info . dependencies peerDependencies devDependencies | grep -i emotionLength of output: 614
Script:
#!/bin/bash # Find all package.json files and grep for emotion-related dependencies echo "=== Searching for @emotion dependencies in package.json files ===" fd package.json --type f --exec grep -l "emotion" {} \; | while read -r file; do echo "=== $file ===" grep "emotion" "$file" || true done echo -e "\n=== Searching for @emotion/css imports ===" rg "@emotion/css" --type ts --type tsx --type js --type jsx -lLength of output: 647
Script:
#!/bin/bash # Search for @emotion/css imports in tsx files specifically echo "=== Searching for @emotion/css imports in tsx files ===" find . -name "*.tsx" -type f -exec grep -l "@emotion/css" {} \; # Double check all package.json files for any emotion-related entries echo -e "\n=== Double checking all package.json files for emotion packages ===" find . -name "package.json" -type f -exec grep -i "emotion" {} \;Length of output: 535
| export const AccordionPromo: React.FC<IAccordionPromoProps> = (props) => { | ||
| const mainClassName = cx(styles[baseClass], styles[promoBaseClass]); | ||
| const mainClassName = cx(styles.baseStyles(true)); | ||
|
|
||
| return ( | ||
| <AccordionComponent mainClassName={mainClassName} isPromo {...props} /> |
There was a problem hiding this comment.
Now we can just pass the class name without cx
<AccordionComponent mainClassName={styles.baseStyles} ...
Resolves: #{issue-number}
Description
Styling the component using the
@emotion/css. Currently, only theAccordioncomponent is affected for the test.Storybook
https://feature-emotion--613a8e945a5665003a05113b.chromatic.com
Checklist
Obligatory:
Summary by CodeRabbit
Dependencies
@emotion/cssas a peer and regular dependencyStyling
styles.tsComponent Updates
index.tsCode Structure