[New Component] Aspect Ratio component #656
Conversation
WalkthroughThe pull request introduces a new Changes
Possibly related issues
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
src/components/ui/AspectRatio/AspectRatio.tsx (1)
1-22: Add accessibility considerationsThe component should handle accessibility for media content:
- Consider adding role="img" when wrapping images
- Ensure ARIA attributes are properly propagated
Would you like me to provide an implementation that includes these accessibility improvements?
🧰 Tools
🪛 eslint
[error] 15-15: Operator '=' must be spaced.
(space-infix-ops)
[error] 15-17: Block must not be padded by blank lines.
(padded-blocks)
[error] 18-18: A space is required after '{'.
(object-curly-spacing)
[error] 18-18: A space is required before '}'.
(object-curly-spacing)
[error] 22-22: Newline required at end of file but not found.
(eol-last)
src/components/ui/AspectRatio/stories/AspectRatio.stories.js (1)
4-25: Enhance story coverageThe current stories only demonstrate the default case. Consider adding:
- Different aspect ratios (4:3, 1:1, etc.)
- Error states with invalid ratios
- Examples with different content types (video, canvas, etc.)
- Responsive behavior examples
Would you like me to provide additional story implementations?
🧰 Tools
🪛 eslint
[error] 12-12: Unexpected tab character.
(no-tabs)
[error] 13-13: Unexpected tab character.
(no-tabs)
[error] 14-14: Unexpected tab character.
(no-tabs)
[error] 15-15: Unexpected tab character.
(no-tabs)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/components/ui/AspectRatio/AspectRatio.tsx(1 hunks)src/components/ui/AspectRatio/stories/AspectRatio.stories.js(1 hunks)src/components/ui/AspectRatio/tests/AspectRatio.test.js(1 hunks)styles/themes/components/aspectratio.scss(1 hunks)styles/themes/default.scss(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- styles/themes/components/aspectratio.scss
- src/components/ui/AspectRatio/tests/AspectRatio.test.js
🧰 Additional context used
🪛 eslint
src/components/ui/AspectRatio/AspectRatio.tsx
[error] 15-15: Operator '=' must be spaced.
(space-infix-ops)
[error] 15-17: Block must not be padded by blank lines.
(padded-blocks)
[error] 18-18: A space is required after '{'.
(object-curly-spacing)
[error] 18-18: A space is required before '}'.
(object-curly-spacing)
[error] 22-22: Newline required at end of file but not found.
(eol-last)
src/components/ui/AspectRatio/stories/AspectRatio.stories.js
[error] 12-12: Unexpected tab character.
(no-tabs)
[error] 13-13: Unexpected tab character.
(no-tabs)
[error] 14-14: Unexpected tab character.
(no-tabs)
[error] 15-15: Unexpected tab character.
(no-tabs)
🔇 Additional comments (1)
styles/themes/default.scss (1)
3-3: Verify aspectratio.scss file existence
The import statement looks correct, but let's verify the file exists and contains the necessary styles.
✅ Verification successful
Import statement is valid and file contains proper styles
The aspectratio.scss file exists at the correct location and contains appropriate styles for handling aspect ratio images with proper class naming convention.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the aspectratio.scss file exists and contains styles
if [ -f "styles/themes/components/aspectratio.scss" ]; then
echo "File exists. Checking content:"
cat "styles/themes/components/aspectratio.scss"
else
echo "Error: aspectratio.scss file not found"
fi
Length of output: 255
|
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/components/ui/AspectRatio/AspectRatio.tsx (1)
20-20: Improve component structure and formattingThe component's return statement needs better formatting and proper spacing.
- return <div style={{aspectRatio:ratio}} className={clsx(rootClass, className)} {...props}>{children} </div> + return ( + <div + style={{ aspectRatio: validatedRatio }} + className={clsx(rootClass, className)} + {...props} + > + {children} + </div> + );🧰 Tools
🪛 eslint
[error] 20-20: A space is required after '{'.
(object-curly-spacing)
[error] 20-20: A space is required before '}'.
(object-curly-spacing)
src/components/ui/AspectRatio/stories/AspectRatio.stories.js (2)
19-25: Add more story variantsThe current stories only show a default 16:9 ratio. Add more variants to demonstrate different use cases.
export const Default = { args: { className:"", ratio: "16/9" } }; + +export const Square = { + args: { + className: "", + ratio: "1" + } +}; + +export const Portrait = { + args: { + className: "", + ratio: "2/3" + } +}; + +export const CustomRatio = { + args: { + className: "", + ratio: "1.85" // Common cinema aspect ratio + } +};
11-14: Fix inconsistent indentationThe code uses a mix of tabs and spaces. Maintain consistent indentation using spaces.
- className="Image" - src="https://images.pexels.com/photos/346529/pexels-photo-346529.jpeg?cs=srgb&dl=pexels-bri-schneiter-28802-346529.jpg&fm=jpg" - alt="Landscape photograph" - /> + className="Image" + src="https://images.pexels.com/photos/346529/pexels-photo-346529.jpeg?cs=srgb&dl=pexels-bri-schneiter-28802-346529.jpg&fm=jpg" + alt="Landscape photograph" + />🧰 Tools
🪛 eslint
[error] 11-11: Unexpected tab character.
(no-tabs)
[error] 12-12: Unexpected tab character.
(no-tabs)
[error] 13-13: Unexpected tab character.
(no-tabs)
[error] 14-14: Unexpected tab character.
(no-tabs)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/components/ui/AspectRatio/AspectRatio.tsx(1 hunks)src/components/ui/AspectRatio/stories/AspectRatio.stories.js(1 hunks)src/components/ui/AspectRatio/tests/AspectRatio.test.js(1 hunks)
🧰 Additional context used
🪛 eslint
src/components/ui/AspectRatio/tests/AspectRatio.test.js
[error] 20-22: Block must not be padded by blank lines.
(padded-blocks)
[error] 22-22: Newline required at end of file but not found.
(eol-last)
src/components/ui/AspectRatio/AspectRatio.tsx
[error] 15-15: Operator '=' must be spaced.
(space-infix-ops)
[error] 15-17: Block must not be padded by blank lines.
(padded-blocks)
[error] 20-20: A space is required after '{'.
(object-curly-spacing)
[error] 20-20: A space is required before '}'.
(object-curly-spacing)
[error] 24-24: Newline required at end of file but not found.
(eol-last)
src/components/ui/AspectRatio/stories/AspectRatio.stories.js
[error] 11-11: Unexpected tab character.
(no-tabs)
[error] 12-12: Unexpected tab character.
(no-tabs)
[error] 13-13: Unexpected tab character.
(no-tabs)
[error] 14-14: Unexpected tab character.
(no-tabs)
🪛 Biome (1.9.4)
src/components/ui/AspectRatio/AspectRatio.tsx
[error] 17-17: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.
(lint/suspicious/noGlobalIsNan)
🔇 Additional comments (3)
src/components/ui/AspectRatio/AspectRatio.tsx (2)
7-13: Fix props type definition
The props property is incorrectly typed as an array of records. For spread props, it should be a single record type.
17-17:
Improve ratio validation logic
The current validation has several issues:
- Uses unsafe
isNaNinstead ofNumber.isNaN - Regex validation could allow invalid numbers (e.g., "0/0")
- Missing validation for negative numbers
- if (isNaN(Number(ratio)) && !ratio.match(/^(\d+)\/(\d+)$/)) ratio = "1"
+ const validateRatio = (ratio: string): string => {
+ if (ratio.includes('/')) {
+ const [numerator, denominator] = ratio.split('/').map(Number);
+ if (!Number.isNaN(numerator) && !Number.isNaN(denominator) &&
+ numerator > 0 && denominator > 0) {
+ return ratio;
+ }
+ } else {
+ const numericRatio = Number(ratio);
+ if (!Number.isNaN(numericRatio) && numericRatio > 0) {
+ return ratio;
+ }
+ }
+ return "1";
+ };
+ const validatedRatio = validateRatio(ratio);Likely invalid or redundant comment.
🧰 Tools
🪛 Biome (1.9.4)
[error] 17-17: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.
(lint/suspicious/noGlobalIsNan)
🪛 eslint
[error] 15-17: Block must not be padded by blank lines.
(padded-blocks)
src/components/ui/AspectRatio/stories/AspectRatio.stories.js (1)
10-15: Replace external image with local asset
Using an external image URL in stories is not recommended.
🧰 Tools
🪛 eslint
[error] 11-11: Unexpected tab character.
(no-tabs)
[error] 12-12: Unexpected tab character.
(no-tabs)
[error] 13-13: Unexpected tab character.
(no-tabs)
[error] 14-14: Unexpected tab character.
(no-tabs)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/components/ui/AspectRatio/AspectRatio.tsx (1)
20-22: Improve component structure and formattingThe component rendering could be improved for better readability and maintainability.
- const rootClass = customClassSwitcher(customRootClass, COMPONENT_NAME); - return <div style={{aspectRatio:ratio}} className={clsx(rootClass, className)} {...props}>{children} </div> + const rootClass = customClassSwitcher(customRootClass, COMPONENT_NAME); + + return ( + <div + style={{ aspectRatio: ratio }} + className={clsx(rootClass, className)} + {...props} + > + {children} + </div> + );🧰 Tools
🪛 eslint
[error] 21-21: A space is required after '{'.
(object-curly-spacing)
[error] 21-21: A space is required before '}'.
(object-curly-spacing)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/components/ui/AspectRatio/AspectRatio.tsx(1 hunks)src/components/ui/AspectRatio/tests/AspectRatio.test.js(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
src/components/ui/AspectRatio/AspectRatio.tsx
[error] 17-17: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.
(lint/suspicious/noGlobalIsNan)
🪛 eslint
src/components/ui/AspectRatio/AspectRatio.tsx
[error] 15-15: Operator '=' must be spaced.
(space-infix-ops)
[error] 15-17: Block must not be padded by blank lines.
(padded-blocks)
[error] 21-21: A space is required after '{'.
(object-curly-spacing)
[error] 21-21: A space is required before '}'.
(object-curly-spacing)
[error] 25-25: Newline required at end of file but not found.
(eol-last)
src/components/ui/AspectRatio/tests/AspectRatio.test.js
[error] 40-43: Block must not be padded by blank lines.
(padded-blocks)
[error] 42-43: More than 1 blank line not allowed.
(no-multiple-empty-lines)
[error] 43-43: Newline required at end of file but not found.
(eol-last)
🔇 Additional comments (2)
src/components/ui/AspectRatio/AspectRatio.tsx (1)
7-13: Fix props type definition
The props type definition needs to be corrected to properly support HTML attributes spreading.
export type AspectRatioProps = {
children: React.ReactNode;
customRootClass?: string;
className?: string;
ratio?: string;
- props: Record<string, any>[];
-}
+} & React.HTMLAttributes<HTMLDivElement>;src/components/ui/AspectRatio/tests/AspectRatio.test.js (1)
5-43: 🛠️ Refactor suggestion
Enhance test coverage and organization
While the basic test coverage is good, there are some improvements needed:
- Missing tests for props spreading
- Could better organize tests with nested describe blocks
- Missing edge cases for ratio validation
describe('AspectRatio', () => {
+ describe('rendering', () => {
test('renders AspectRatio component', () => {
render(<AspectRatio>Content</AspectRatio>);
expect(screen.getByText('Content')).toBeInTheDocument();
});
test('applies custom classes correctly', () => {
render(<AspectRatio className="additional-class">Content</AspectRatio>);
const divElement = screen.getByText('Content');
expect(divElement).toHaveClass('additional-class');
});
+
+ test('spreads additional props to div element', () => {
+ render(
+ <AspectRatio data-testid="aspect-ratio" aria-label="container">
+ Content
+ </AspectRatio>
+ );
+ const element = screen.getByTestId('aspect-ratio');
+ expect(element).toHaveAttribute('aria-label', 'container');
+ });
+ });
+ describe('ratio validation', () => {
test('applies correct aspect ratio', () => {
render(<AspectRatio ratio="16/9">Content</AspectRatio>);
expect(screen.getByText('Content').style.aspectRatio).toBe('16/9');
});
test('applies correct aspect ratio when ratio is not provided', () => {
render(<AspectRatio>Content</AspectRatio>);
expect(screen.getByText('Content').style.aspectRatio).toBe('1');
});
+ test('handles decimal ratios', () => {
+ render(<AspectRatio ratio="1.78">Content</AspectRatio>);
+ expect(screen.getByText('Content').style.aspectRatio).toBe('1.78');
+ });
+
test('applies correct aspect ratio when ratio is invalid', () => {
render(<AspectRatio ratio="invalid">Content</AspectRatio>);
expect(screen.getByText('Content').style.aspectRatio).toBe('1');
});
test('applies correct aspect ratio when ratio contains a character', () => {
render(<AspectRatio ratio="16/o">Content</AspectRatio>);
expect(screen.getByText('Content').style.aspectRatio).toBe('1');
});
test('applies correct aspect ratio when ratio is negative', () => {
render(<AspectRatio ratio="-5">Content</AspectRatio>);
expect(screen.getByText('Content').style.aspectRatio).toBe('1');
});
+
+ test('handles zero denominator in ratio', () => {
+ render(<AspectRatio ratio="16/0">Content</AspectRatio>);
+ expect(screen.getByText('Content').style.aspectRatio).toBe('1');
+ });
+ });
});Likely invalid or redundant comment.
🧰 Tools
🪛 eslint
[error] 40-43: Block must not be padded by blank lines.
(padded-blocks)
[error] 42-43: More than 1 blank line not allowed.
(no-multiple-empty-lines)
[error] 43-43: Newline required at end of file but not found.
(eol-last)
| const AspectRatio = ({ children, customRootClass, className, ratio="1", ...props }: AspectRatioProps) => { | ||
|
|
||
| if (isNaN(Number(ratio)) && !ratio.match(/^(\d+)\/(\d+)$/)) ratio = "1" | ||
| if (Number(ratio) <= 0) ratio = "1" |
There was a problem hiding this comment.
Improve ratio validation and parameter handling
The current ratio validation has several issues:
- Uses unsafe
isNaNinstead ofNumber.isNaN - Mutates the parameter directly
- Could be more robust in handling edge cases
-const AspectRatio = ({ children, customRootClass, className, ratio="1", ...props }: AspectRatioProps) => {
+const AspectRatio = ({
+ children,
+ customRootClass,
+ className,
+ ratio: initialRatio = "1",
+ ...props
+}: AspectRatioProps) => {
+ const validateRatio = (value: string): string => {
+ if (value.includes('/')) {
+ const [numerator, denominator] = value.split('/').map(Number);
+ if (!Number.isNaN(numerator) && !Number.isNaN(denominator) && denominator > 0) {
+ return value;
+ }
+ } else {
+ const numericValue = Number(value);
+ if (!Number.isNaN(numericValue) && numericValue > 0) {
+ return value;
+ }
+ }
+ return "1";
+ };
+ const ratio = validateRatio(initialRatio);
- if (isNaN(Number(ratio)) && !ratio.match(/^(\d+)\/(\d+)$/)) ratio = "1"
- if (Number(ratio) <= 0) ratio = "1"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const AspectRatio = ({ children, customRootClass, className, ratio="1", ...props }: AspectRatioProps) => { | |
| if (isNaN(Number(ratio)) && !ratio.match(/^(\d+)\/(\d+)$/)) ratio = "1" | |
| if (Number(ratio) <= 0) ratio = "1" | |
| const AspectRatio = ({ | |
| children, | |
| customRootClass, | |
| className, | |
| ratio: initialRatio = "1", | |
| ...props | |
| }: AspectRatioProps) => { | |
| const validateRatio = (value: string): string => { | |
| if (value.includes('/')) { | |
| const [numerator, denominator] = value.split('/').map(Number); | |
| if (!Number.isNaN(numerator) && !Number.isNaN(denominator) && denominator > 0) { | |
| return value; | |
| } | |
| } else { | |
| const numericValue = Number(value); | |
| if (!Number.isNaN(numericValue) && numericValue > 0) { | |
| return value; | |
| } | |
| } | |
| return "1"; | |
| }; | |
| const ratio = validateRatio(initialRatio); |
🧰 Tools
🪛 Biome (1.9.4)
[error] 17-17: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.
(lint/suspicious/noGlobalIsNan)
🪛 eslint
[error] 15-15: Operator '=' must be spaced.
(space-infix-ops)
[error] 15-17: Block must not be padded by blank lines.
(padded-blocks)
| import { render, screen } from '@testing-library/react'; | ||
| import AspectRatio from '../AspectRatio'; | ||
|
|
||
| describe('AspectRatio', () => { |
kotAPI
added
the
automerge
#526
Summary by CodeRabbit
New Features
AspectRatiocomponent to manage the aspect ratio of child elements.AspectRatiocomponent.Bug Fixes
ratioprop to ensure proper formatting and default behavior.Tests
AspectRatiocomponent to verify rendering, class application, and aspect ratio styling under various scenarios.