560-refactor: Widget principles #636
Conversation
github-actions
bot
changed the title
|
Lighthouse Report:
|
src/widgets/principles/ui/principle-card/principle-card.module.scss
Outdated
Show resolved
Hide resolved
|
Lighthouse Report:
|
Quiddlee
requested review from
andron13,
dzmitry-varabei,
ansivgit and
natanchik
as code owners
📝 Walkthrough📝 WalkthroughWalkthroughThis pull request introduces several changes across multiple files, primarily focusing on the Changes
Assessment against linked issues
Possibly related issues
Suggested reviewers
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 eslint
dev-data/index.tsOops! Something went wrong! :( ESLint: 8.57.1 ESLint couldn't find the plugin "eslint-plugin-boundaries". (The package "eslint-plugin-boundaries" was not found when loaded as a Node module from the directory "".) It's likely that the plugin isn't installed correctly. Try reinstalling by running the following: The plugin "eslint-plugin-boundaries" was referenced from the config file in " » @feature-sliced/eslint-config » /node_modules/@feature-sliced/eslint-config/rules/layers-slices". If you still can't figure out the problem, please stop by https://eslint.org/chat/help to chat with the team. 📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
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
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (8)
src/widgets/principles/index.ts (2)
1-1: Remove the.tsfile extension from the import path.TypeScript resolves
.tsextensions automatically, and including them is not a common practice.-export type { PrincipleCard } from './types.ts'; +export type { PrincipleCard } from './types';
2-2: Consider simplifying the component path.The export path looks good, but contains a redundant "principles" segment. Consider simplifying it.
-export { Principles } from './ui/principles/principles'; +export { Principles } from './ui/principles';src/widgets/principles/types.ts (1)
3-7: Consider adding JSDoc comments for better documentation.The type definition is clean and well-structured. To improve maintainability, consider adding JSDoc comments to document the purpose of each property.
+/** + * Represents a principle card with title, description, and icon. + */ export type PrincipleCard = { + /** The title of the principle card */ title: string; + /** The description explaining the principle */ description: string; + /** The icon to be displayed with the principle */ icon: ReactNode; };src/widgets/principles/ui/principle-card/principle-card.tsx (1)
10-15: Consider adding error boundary for icon rendering.While the implementation is solid with semantic HTML and proper component composition, the direct rendering of the
iconprop could benefit from error handling.- <span className={cx('icon')}>{icon}</span> + <span className={cx('icon')}> + {React.isValidElement(icon) ? icon : null} + </span>src/widgets/principles/ui/principles/principles.tsx (1)
3-3: Remove file extension from import pathThe
.tsxextension in the import statement is unnecessary and non-standard in TypeScript/React projects.-import { PrincipleCard } from '@/widgets/principles/ui/principle-card/principle-card.tsx'; +import { PrincipleCard } from '@/widgets/principles/ui/principle-card/principle-card';src/widgets/principles/ui/principles/principles.test.tsx (1)
Line range hint
20-26: Consider enhancing test coverageWhile the basic visibility checks are good, consider adding assertions for:
- Correct order of cards
- Proper HTML structure
- Accessibility attributes
principleCards.forEach(({ title, description }, index) => { const titleElement = screen.getByText(title); const descriptionElement = screen.getByText(description); expect(titleElement).toBeVisible(); expect(descriptionElement).toBeVisible(); + expect(titleElement).toHaveAttribute('role', 'heading'); + const card = screen.getAllByTestId('principle-card')[index]; + expect(card).toContainElement(titleElement); + expect(card).toContainElement(descriptionElement); });dev-data/principle-cards.data.tsx (1)
12-12: Add width and height props to Image componentsWhile the Image implementation is good, adding explicit dimensions would prevent layout shifts during loading.
- icon: <Image src={openToEveryoneIcon} alt="" aria-hidden="true" />, + icon: <Image src={openToEveryoneIcon} alt="" aria-hidden="true" width={24} height={24} />,Also applies to: 18-18, 24-24
src/widgets/principles/ui/principle-card/principle-card.module.scss (1)
60-81: Consider extracting magic numbers to variables.The icon dimensions and blur values should be defined as SCSS variables for consistency and maintainability.
+ $icon-size: 44px; + $accent-size: 20px; + $accent-blur: 8px; .icon { img { - width: 44px; - height: 44px; + width: $icon-size; + height: $icon-size; } &::before { - width: 20px; - height: 20px; + width: $accent-size; + height: $accent-size; filter: blur($accent-blur); } }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (14)
dev-data/index.ts(1 hunks)dev-data/principle-cards.data.tsx(1 hunks)src/shared/icons/index.tsx(0 hunks)src/shared/icons/open-source-philosophy-icon.tsx(0 hunks)src/shared/icons/open-to-everyone-icon.tsx(0 hunks)src/shared/icons/teach-It-forward-icon.tsx(0 hunks)src/widgets/principles/index.ts(1 hunks)src/widgets/principles/types.ts(1 hunks)src/widgets/principles/ui/principle-card/principle-card.module.scss(4 hunks)src/widgets/principles/ui/principle-card/principle-card.tsx(1 hunks)src/widgets/principles/ui/principle-card/principles.tsx(0 hunks)src/widgets/principles/ui/principles/principles.module.scss(1 hunks)src/widgets/principles/ui/principles/principles.test.tsx(3 hunks)src/widgets/principles/ui/principles/principles.tsx(1 hunks)
💤 Files with no reviewable changes (5)
- src/shared/icons/index.tsx
- src/shared/icons/open-source-philosophy-icon.tsx
- src/shared/icons/open-to-everyone-icon.tsx
- src/shared/icons/teach-It-forward-icon.tsx
- src/widgets/principles/ui/principle-card/principles.tsx
✅ Files skipped from review due to trivial changes (1)
- src/widgets/principles/ui/principles/principles.module.scss
🔇 Additional comments (11)
src/widgets/principles/types.ts (1)
1-1: LGTM! Clean and necessary import.
The ReactNode import is appropriate for typing the icon property.
src/widgets/principles/ui/principle-card/principle-card.tsx (1)
1-8: LGTM! Well-structured imports and setup.
The imports are well-organized, utilizing shared UI components and proper CSS module binding.
src/widgets/principles/ui/principles/principles.tsx (1)
10-23: LGTM! Well-structured component implementation
The component follows React best practices with:
- Proper semantic HTML usage
- Efficient list rendering with unique keys
- Clean CSS module implementation
src/widgets/principles/ui/principles/principles.test.tsx (1)
3-4: LGTM: Import changes align with restructuring
The updated imports correctly reflect the new data source structure.
dev-data/principle-cards.data.tsx (2)
1-5: LGTM: Clean import organization
The switch to SVG assets with Next.js Image component is a good optimization choice.
7-7: LGTM: Improved type and naming
The rename to principleCards and type update to PrincipleCard improve code clarity.
src/widgets/principles/ui/principle-card/principle-card.module.scss (4)
7-9: LGTM: Grid layout improves structure.
The switch to grid layout with explicit column sizing provides better structure and maintainability.
20-33: Consider performance impact of blur effect.
The blur effect using filter: blur(32px) on a large element (310x300px) might impact rendering performance on lower-end devices.
Consider:
- Reducing the blur radius
- Using a smaller element size
- Adding
will-change: filterif animations are involved
48-51: Skip z-index review.
The z-index usage has been previously discussed and justified with visual evidence.
111-114: LGTM: Responsive adjustments are appropriate.
The font-size and line-height adjustments for laptop viewports are reasonable.
dev-data/index.ts (1)
58-58: LGTM! Clean export statement.
The new export for principleCards is properly placed and follows the established patterns in the file.
|
Lighthouse Report:
|
|
Lighthouse Report:
|
What type of PR is this? (select all that apply)
Description
Related Tickets & Documents
Screenshots, Recordings
Added/updated tests?
[optional] Are there any post deployment tasks we need to perform?
[optional] What gif best describes this PR or how it makes you feel?
Summary by CodeRabbit
Principlescomponent to display a list of principle cards.PrincipleCardto standardize card properties.