feat(ActionCard): add loading state with optional animation #1507
Conversation
WalkthroughThis pull request introduces a loading state for the Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
✨ 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 (
|
There was a problem hiding this comment.
Copilot reviewed 4 out of 7 changed files in this pull request and generated no comments.
Files not reviewed (3)
- packages/react-components/src/components/ActionCard/ActionCard.module.scss: Language not supported
- packages/react-components/src/components/Skeleton/Skeleton.module.scss: Language not supported
- packages/react-components/src/styles/_animations.scss: Language not supported
Comments suppressed due to low confidence (1)
packages/react-components/src/components/ActionCard/ActionCard.spec.tsx:77
- The test case should also check for the presence of the loading indicator to ensure the loading state is correctly displayed.
it('should display loading state when isLoading is true', () => {
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
packages/react-components/src/components/ActionCard/ActionCard.tsx (3)
57-59: Enhance accessibility during loading state.Consider adding
aria-busy="true"when loading to improve screen reader experience.role={isLoading ? 'presentation' : 'button'} aria-label="Action Card" +aria-busy={isLoading} tabIndex={isLoading ? -1 : 0}
29-29: Extract loading state check to a helper function.The loading check is duplicated in both handlers.
+const isInteractionDisabled = () => isLoading; + const handleOnClick = (e: MouseEvent<HTMLDivElement>) => { - if (isLoading) return; + if (isInteractionDisabled()) return; // ... }; const handleOnKeyDown = (e: KeyboardEvent<HTMLDivElement>) => { - if (isLoading) return; + if (isInteractionDisabled()) return; // ... };Also applies to: 38-38
Line range hint
44-48: Use constants for keyboard keys.Define keyboard keys as constants to avoid magic strings and improve maintainability.
+const KEYBOARD_KEYS = { + ENTER: 'Enter', + SPACE: ' ', + SPACEBAR: 'Spacebar', + SPACE_LEGACY: 'Space', +} as const; + if ( - e.key === 'Enter' || - e.key === ' ' || - e.key === 'Spacebar' || - e.key === 'Space' + e.key === KEYBOARD_KEYS.ENTER || + e.key === KEYBOARD_KEYS.SPACE || + e.key === KEYBOARD_KEYS.SPACEBAR || + e.key === KEYBOARD_KEYS.SPACE_LEGACY )packages/react-components/src/components/ActionCard/ActionCard.spec.tsx (1)
77-84: Add more test coverage for loading states.Consider adding tests for:
- Animated loading state
- Interaction prevention during loading
- Accessibility attributes
it('should prevent interactions when loading', () => { const onClick = vi.fn(); const { getByRole } = renderComponent({ onClick, isLoading: true }); userEvent.click(getByRole('presentation')); expect(onClick).not.toHaveBeenCalled(); }); it('should set correct accessibility attributes when loading', () => { const { getByRole } = renderComponent({ isLoading: true }); const card = getByRole('presentation'); expect(card).toHaveAttribute('aria-busy', 'true'); expect(card).toHaveAttribute('tabIndex', '-1'); });packages/react-components/src/styles/_animations.scss (2)
1-9: Consider adding easing for smoother animation.The linear movement might appear mechanical. Adding easing could create a more polished effect.
@keyframes loading { 0% { left: -100%; + animation-timing-function: ease-in-out; } 100% { left: 100%; } }
11-29: Add fallback values for CSS variables.The gradient colors rely on CSS variables without fallbacks, which could cause issues if variables are undefined.
background: linear-gradient( 90deg, - var(--animated-gradient-value-1), - var(--animated-gradient-value-2), - var(--animated-gradient-value-3) + var(--animated-gradient-value-1, #f0f0f0), + var(--animated-gradient-value-2, #e0e0e0), + var(--animated-gradient-value-3, #f0f0f0) );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
packages/react-components/src/components/ActionCard/ActionCard.module.scss(2 hunks)packages/react-components/src/components/ActionCard/ActionCard.spec.tsx(1 hunks)packages/react-components/src/components/ActionCard/ActionCard.stories.tsx(2 hunks)packages/react-components/src/components/ActionCard/ActionCard.tsx(3 hunks)packages/react-components/src/components/ActionCard/types.ts(1 hunks)packages/react-components/src/components/Skeleton/Skeleton.module.scss(3 hunks)packages/react-components/src/styles/_animations.scss(1 hunks)
🔇 Additional comments (5)
packages/react-components/src/components/ActionCard/types.ts (1)
24-31: Well-documented props with clear types!The new loading state props are properly typed as optional booleans with clear JSDoc comments.
packages/react-components/src/components/ActionCard/ActionCard.stories.tsx (1)
82-97: Great story implementation showcasing both loading states!The story effectively demonstrates both regular and animated loading states using StoryDescriptor for clear organization.
packages/react-components/src/components/ActionCard/ActionCard.tsx (1)
18-19: Props initialized with sensible defaults!The loading props default to false, which is the expected behavior.
packages/react-components/src/components/Skeleton/Skeleton.module.scss (1)
30-30: Clean implementation using the new mixin!Good job reusing the skeleton-loading mixin consistently across components.
Also applies to: 41-41
packages/react-components/src/components/ActionCard/ActionCard.module.scss (1)
32-34: Clean implementation of animated loading state!The skeleton-loading mixin is applied correctly.
packages/react-components/src/components/ActionCard/ActionCard.module.scss
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/react-components/src/components/ActionCard/ActionCard.tsx (2)
18-26: Extract loading class name prefix to a constant.Consider extracting the repeated
${baseClass}--loadingprefix to improve maintainability.const baseClass = 'action-card'; +const loadingClass = `${baseClass}--loading`; export const ActionCard: FC<PropsWithChildren<ActionCardProps>> = ({ // ...props }) => { const wrapperClassNames = cx(styles[`main-wrapper`], { - [styles[`${baseClass}--loading`]]: isLoading, - [styles[`${baseClass}--loading--animated`]]: isLoadingAnimated, + [styles[loadingClass]]: isLoading, + [styles[`${loadingClass}--animated`]]: isLoadingAnimated, });
56-63: Enhance the aria-label for better accessibility.The current "Action Card" label is generic. Consider making it more descriptive of the card's purpose.
- aria-label="Action Card" + aria-label={`${children} action card`}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/react-components/src/components/ActionCard/ActionCard.module.scss(2 hunks)packages/react-components/src/components/ActionCard/ActionCard.tsx(3 hunks)packages/react-components/src/components/ActionCard/types.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/react-components/src/components/ActionCard/ActionCard.module.scss
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build
- GitHub Check: chromatic-deployment
🔇 Additional comments (3)
packages/react-components/src/components/ActionCard/types.ts (1)
Line range hint
3-45: Well-structured discriminated union type!The type definition effectively enforces the relationship between
isLoadingandisLoadingAnimatedprops, preventing invalid combinations.packages/react-components/src/components/ActionCard/ActionCard.tsx (2)
29-29: Good defensive programming!Early returns during loading state effectively prevent unwanted interactions.
Also applies to: 38-38
68-92: Clean conditional rendering implementation!Content is properly hidden during loading while maintaining the component structure.
Resolves: #1234
Description
This pull request introduces a loading state to the
ActionCardcomponent, including both default and animated loading styles. It also refactors the existing skeleton loading animation into a reusable mixin.Storybook
https://feature-1234--613a8e945a5665003a05113b.chromatic.com
Checklist
Obligatory:
Summary by CodeRabbit
Release Notes: ActionCard Loading State Enhancement
New Features
isLoadingandisLoadingAnimatedprops.Improvements
Testing