561-refactor: Fsd widget required #682
Conversation
github-actions
bot
changed the title
|
run visual now |
|
Lighthouse Report:
|
|
Lighthouse Report:
|
|
run visual now |
|
run visual now |
|
Lighthouse Report:
|
|
Lighthouse Report:
|
|
Lighthouse Report:
|
There was a problem hiding this comment.
Copilot reviewed 16 out of 31 changed files in this pull request and generated no comments.
Files not reviewed (15)
- src/app/docs/components/search/search.module.scss: Language not supported
- src/core/base-layout/components/header/dropdown/dropdown-wrapper.module.scss: Language not supported
- src/core/base-layout/components/header/nav-item/nav-item.module.scss: Language not supported
- src/core/styles/_mixins.scss: Language not supported
- src/core/styles/normalize.scss: Language not supported
- src/entities/course/ui/course-card/course-card.module.scss: Language not supported
- src/entities/event/ui/event-card/event-card.module.scss: Language not supported
- src/entities/trainer/ui/trainers-card/trainer-card.module.scss: Language not supported
- src/shared/ui/social-media-item/social-media-item.module.scss: Language not supported
- src/widgets/about-course/ui/about-course-grid/about-course-grid.module.scss: Language not supported
- src/widgets/benefits/ui/benefits/benefits.module.scss: Language not supported
- src/widgets/communication/ui/communication.module.scss: Language not supported
- src/widgets/member-activity/ui/stage-card/step/step.scss: Language not supported
- src/views/aws-developer.tsx: Evaluated as low risk
- src/views/aws-devops.tsx: Evaluated as low risk
SpaNb4
requested review from
andron13,
dzmitry-varabei and
natanchik
as code owners
📝 WalkthroughWalkthroughThis pull request makes several refactoring changes. It updates import paths for types (e.g., Course and LinkList) by replacing references to the old required.types path with a new one. Multiple SCSS files are modified through removals, reordering, or reintroduction of style properties. In addition, the widget Required is refactored: view files no longer pass extra boolean props and the old Required component is removed in favor of a newly implemented one in the required subfolder. A new CourseModuleElement component is also introduced to modularize course module display. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant R as Required Component
participant D as courseDataMap
participant WT as WidgetTitle
participant L as List
participant CME as CourseModuleElement
U->>R: Provide courseName prop
R->>D: Retrieve course data for courseName
D-->>R: Return { title, knowBefore, willLearn }
alt knowBefore exists
R->>WT: Render knowBefore section
R->>L: Render knowBefore list
end
alt willLearn exists
loop each willLearn item
R->>CME: Render CourseModuleElement
end
end
R-->>U: Rendered UI
sequenceDiagram
participant U as User
participant CME as CourseModuleElement
participant S as Subtitle
participant L as List
U->>CME: Provide courseModule prop
CME->>S: Render courseModule.title
CME->>L: Render courseModule.description list
CME-->>U: Rendered module element
Assessment against linked issues
Possibly related PRs
Suggested reviewers
✨ Finishing Touches
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
🧹 Nitpick comments (2)
src/widgets/required/types.ts (1)
1-21: Consider adding JSDoc comments for better documentation.The type hierarchy is well-structured and follows TypeScript best practices. Consider adding JSDoc comments to document the purpose of each type, especially for the exported ones.
+/** Represents an item with a clickable link and associated text */ type ItemWithLink = { id: number; text: string; title: string; link: string; }; +/** List of items with links */ export type LinkList = ItemWithLink[]; type Description = (string | LinkList)[]; +/** Represents a module within a course with a title and description */ export type CourseModule = { title: string; description: Description; }; +/** Represents a course with optional prerequisites and learning objectives */ export type Course = { title: string; knowBefore?: CourseModule; willLearn?: CourseModule[]; };src/widgets/required/ui/course-module/course-module.tsx (1)
15-26: Consider adding optional className prop for better customization.The component is well-structured but could benefit from allowing custom class names.
type CourseModuleElementProps = { courseModule: CourseModule; + className?: string; }; -export function CourseModuleElement({ courseModule }: CourseModuleElementProps) { +export function CourseModuleElement({ courseModule, className }: CourseModuleElementProps) { const { title, description } = courseModule; return ( - <article className={cx('course-module-element')}> + <article className={cx('course-module-element', className)}>
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (31)
dev-data/courseTitles.data.ts(1 hunks)src/app/docs/components/search/search.module.scss(1 hunks)src/core/base-layout/components/header/dropdown/dropdown-wrapper.module.scss(1 hunks)src/core/base-layout/components/header/nav-item/nav-item.module.scss(1 hunks)src/core/styles/_mixins.scss(1 hunks)src/core/styles/normalize.scss(1 hunks)src/entities/course/ui/course-card/course-card.module.scss(2 hunks)src/entities/event/ui/event-card/event-card.module.scss(3 hunks)src/entities/trainer/ui/trainers-card/trainer-card.module.scss(1 hunks)src/shared/types.ts(1 hunks)src/shared/ui/social-media-item/social-media-item.module.scss(1 hunks)src/shared/ui/text-with-link/text-with-link.tsx(2 hunks)src/views/angular.tsx(1 hunks)src/views/aws-developer.tsx(1 hunks)src/views/aws-devops.tsx(1 hunks)src/views/aws-fundamentals.tsx(1 hunks)src/widgets/about-course/ui/about-course-grid/about-course-grid.module.scss(1 hunks)src/widgets/benefits/ui/benefits/benefits.module.scss(2 hunks)src/widgets/communication/ui/communication.module.scss(1 hunks)src/widgets/member-activity/ui/stage-card/step/step.scss(1 hunks)src/widgets/places/ui/places/places.module.scss(0 hunks)src/widgets/principles/ui/principle-card/principle-card.module.scss(3 hunks)src/widgets/required/index.ts(1 hunks)src/widgets/required/types.ts(1 hunks)src/widgets/required/ui/course-module/course-module.module.scss(1 hunks)src/widgets/required/ui/course-module/course-module.tsx(1 hunks)src/widgets/required/ui/required.scss(0 hunks)src/widgets/required/ui/required.tsx(0 hunks)src/widgets/required/ui/required/required.module.scss(1 hunks)src/widgets/required/ui/required/required.test.tsx(1 hunks)src/widgets/required/ui/required/required.tsx(1 hunks)
💤 Files with no reviewable changes (3)
- src/widgets/places/ui/places/places.module.scss
- src/widgets/required/ui/required.scss
- src/widgets/required/ui/required.tsx
✅ Files skipped from review due to trivial changes (18)
- src/widgets/about-course/ui/about-course-grid/about-course-grid.module.scss
- src/widgets/required/index.ts
- dev-data/courseTitles.data.ts
- src/widgets/member-activity/ui/stage-card/step/step.scss
- src/widgets/communication/ui/communication.module.scss
- src/entities/course/ui/course-card/course-card.module.scss
- src/core/base-layout/components/header/dropdown/dropdown-wrapper.module.scss
- src/shared/ui/social-media-item/social-media-item.module.scss
- src/shared/types.ts
- src/core/styles/normalize.scss
- src/core/styles/_mixins.scss
- src/core/base-layout/components/header/nav-item/nav-item.module.scss
- src/widgets/benefits/ui/benefits/benefits.module.scss
- src/entities/trainer/ui/trainers-card/trainer-card.module.scss
- src/shared/ui/text-with-link/text-with-link.tsx
- src/widgets/required/ui/course-module/course-module.module.scss
- src/entities/event/ui/event-card/event-card.module.scss
- src/widgets/principles/ui/principle-card/principle-card.module.scss
🔇 Additional comments (9)
src/widgets/required/types.ts (1)
10-10: Verify removal of export for Description type.The removal of the export modifier for the Description type is a good encapsulation practice since it's only used internally by CourseModule. However, let's verify that this change doesn't break any existing imports.
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Search for any remaining imports of the Description type rg "import.*Description.*from.*required/types"Length of output: 48
I'm running an enhanced search to confirm that there are no remaining imports of the Description type. Please allow a moment for the updated check.
🏁 Script executed:
#!/bin/bash # Search for any import statements referencing the Description type rg -n "import\s*\{[^}]*\bDescription\b[^}]*\}\s*from\s*['\"]src/widgets/required/types['\"]"Length of output: 98
Final Verification: Removal of the export for the Description type is safe.
No external imports of Description were found, confirming this internal refactoring aligns with our encapsulation goals.
src/widgets/required/ui/course-module/course-module.tsx (1)
11-13: LGTM! Well-defined prop type.The CourseModuleElementProps type is clear and focused.
src/views/aws-devops.tsx (1)
24-24: LGTM! Simplified Required component usage.Removal of marked1 prop aligns with the refactoring objectives.
src/views/aws-fundamentals.tsx (1)
23-23: LGTM! Consistent with other view changes.Removal of marked2 prop maintains consistency with the refactoring pattern.
src/views/aws-developer.tsx (1)
28-28: LGTM! Verify prop removal across all views.The change maintains consistency with other views.
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Verify Required component usage is consistent across all views rg -l '<Required.*marked[12]' src/views/Length of output: 42
I'll run an additional script to view all usages of the Required component in src/views and check if any instance still includes "marked" properties (e.g., marked1 or marked2).
🏁 Script executed:
#!/bin/bash echo "Searching for all Required component usages in src/views:" rg '<Required ' src/views/ > usage.txt && cat usage.txt echo "Checking if any of these lines include 'marked':" grep -E 'marked1|marked2' usage.txt || echo "No 'marked1' or 'marked2' props found."Length of output: 984
Verification Complete: Removal of marked props is consistent across views.
- The script confirms that no instances of "marked1" or "marked2" are present in any Usage of the Required component.
- All views use the component in the form
<Required courseName={courseName} />.src/views/angular.tsx (1)
31-31: LGTM! Prop removal aligns with widget refactor.The removal of the
marked1prop simplifies the component's interface.src/widgets/required/ui/required/required.test.tsx (1)
4-4: LGTM! Import path updated correctly.The import path change aligns with the new component structure.
src/widgets/required/ui/required/required.module.scss (1)
1-26: LGTM! Well-structured responsive layout.The SCSS implementation follows best practices with appropriate breakpoints and nesting.
src/app/docs/components/search/search.module.scss (1)
11-14: .search-input Styling Update.
The border property has been removed and an outline-offset set to -2px has been added. Please verify visually that the absence of a border and the new focus offset maintain a harmonious design with the rest of the UI.
| {willLearn.map((willLearn, index) => ( | ||
| <CourseModuleElement key={index} courseModule={willLearn} /> | ||
| ))} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid using array indices as React keys.
Using array indices as keys can lead to issues if items are reordered.
Consider adding unique identifiers to your willLearn items or using a stable unique property if available.
| const isKnowBeforeExist = knowBefore && Boolean(knowBefore.description.length); | ||
| const isWillLearnExist = willLearn && Boolean(willLearn.length); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace Boolean coercion with explicit checks.
Based on team's previous discussions, we should avoid implicit coercion.
- const isKnowBeforeExist = knowBefore && Boolean(knowBefore.description.length);
- const isWillLearnExist = willLearn && Boolean(willLearn.length);
+ const isKnowBeforeExist = knowBefore && knowBefore.description.length > 0;
+ const isWillLearnExist = willLearn && willLearn.length > 0;📝 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 isKnowBeforeExist = knowBefore && Boolean(knowBefore.description.length); | |
| const isWillLearnExist = willLearn && Boolean(willLearn.length); | |
| const isKnowBeforeExist = knowBefore && knowBefore.description.length > 0; | |
| const isWillLearnExist = willLearn && willLearn.length > 0; |
There was a problem hiding this comment.
I suggest to change dev-data/required.data.ts like
| .course-module-element { | ||
| margin-top: 20px; | ||
|
|
||
| &:first-child { | ||
| margin-top: 0; | ||
| } | ||
|
|
||
| .course-module-title { | ||
| margin-bottom: 8px; | ||
| } | ||
| } |
There was a problem hiding this comment.
| .course-module-element { | |
| margin-top: 20px; | |
| &:first-child { | |
| margin-top: 0; | |
| } | |
| .course-module-title { | |
| margin-bottom: 8px; | |
| } | |
| } | |
| .course-module-element { | |
| display: flex; | |
| flex-direction: column; | |
| gap: 8px; | |
| } |
| gap: 8rem; | ||
|
|
||
| .course-module-elements-column { | ||
| flex: 1; |
There was a problem hiding this comment.
Do we really need flex: 1 here?
| flex: 1; | |
| display: flex; | |
| flex-direction: column; | |
| gap: 24px; |
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
New Features
Style