763-refactor: Display mentorship dates #789
Conversation
karlovich
requested review from
andron13,
dzmitry-varabei,
SpaNb4,
Quiddlee,
ansivgit and
natanchik
as code owners
github-actions
bot
changed the title
π WalkthroughWalkthroughThe changes enhance the course data model by adding two new nullable properties, Changes
Possibly related PRs
Suggested labels
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: 3
π§Ή Nitpick comments (7)
src/views/mentorship/ui/mentorship-courses/mentorship-courses.tsx (1)
23-28: Make mentoring date display configurable.Consider deriving
showMentoringStartDatefrom course data instead of hardcoding it totrue.<CourseCard key={course.id} className={cx('mentorship-course-card')} {...course} - showMentoringStartDate={true} + showMentoringStartDate={Boolean(course.personalMentoringStartDate)} />src/shared/constants.ts (1)
7-12: Add internationalization support for labels.The app supports multiple languages, but the new labels are only in English.
export const LABELS = { START_DATE: 'Start:', + START_DATE_RU: 'ΠΠ°ΡΠ°Π»ΠΎ:', MENTOR_ACTIVITIES: 'Mentor activities:', + MENTOR_ACTIVITIES_RU: 'ΠΠ΅Π½ΡΠΎΡΡΠΊΠ°Ρ Π°ΠΊΡΠΈΠ²Π½ΠΎΡΡΡ:', MENTORING_DATES_SEPARATOR: '-', SHORT_INFO_SEPARATOR: 'β’', };src/shared/ui/date-simple/date-simple.tsx (1)
21-27: Consider memoizing the component for better performance.Since this component might be rendered frequently in lists, consider using React.memo to prevent unnecessary re-renders.
-export const DateSimple = ({ +export const DateSimple = React.memo(({ startDate, endDate, label, labelSeparator, children, -}: DateStartProps) => { +}: DateStartProps) => {src/entities/course/ui/course-item/course-item.test.tsx (1)
26-36: Add test cases for edge scenarios.Consider adding tests for:
- Null dates
- TO_BE_DETERMINED dates
- Invalid date strings
+ it('handles null dates gracefully', () => { + renderWithRouter(<CourseItem {...mockedProps} startDate={null} />); + expect(screen.queryByTestId('date-time-start')).not.toBeInTheDocument(); + });src/shared/ui/short-info-panel/short-info-panel.tsx (1)
59-59: Add null check for mode prop.The mode prop is optional but used directly in JSX without a null check.
- <span className={cx('mode')}>{mode}</span> + <span className={cx('mode')}>{mode || ''}</span>src/shared/ui/short-info-panel/short-info-panel.test.tsx (1)
10-60: Add test for withMargin prop.The test suite is comprehensive but missing coverage for the withMargin prop's effect on styling.
Consider adding:
it('applies margin class when withMargin is true', () => { render( <ShortInfoPanel startDate="" registrationEndDate="" language="en" withMargin={true} /> ); expect(screen.getByRole('region')).toHaveClass('margin'); });src/shared/ui/date-simple/date-simple.test.tsx (1)
60-62: Remove unnecessary Fragment.The Fragment is redundant as it wraps only a single span element.
- <> <span>{additionalInfoText}</span> - </> + <span>{additionalInfoText}</span>π§° Tools
πͺ Biome (1.9.4)
[error] 60-62: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragment(lint/complexity/noUselessFragments)
π Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
π Files selected for processing (26)
dev-data/courses.data.ts(9 hunks)src/entities/course/helpers/sync-with-api-data.ts(1 hunks)src/entities/course/types.ts(2 hunks)src/entities/course/ui/course-card/course-card.test.tsx(1 hunks)src/entities/course/ui/course-card/course-card.tsx(4 hunks)src/entities/course/ui/course-item/course-item.module.scss(1 hunks)src/entities/course/ui/course-item/course-item.test.tsx(1 hunks)src/entities/course/ui/course-item/course-item.tsx(1 hunks)src/shared/__tests__/constants.ts(8 hunks)src/shared/constants.ts(1 hunks)src/shared/helpers/getActualData.test.ts(1 hunks)src/shared/helpers/getCourseDate.ts(1 hunks)src/shared/ui/date-lang/date-lang.tsx(0 hunks)src/shared/ui/date-lang/index.ts(0 hunks)src/shared/ui/date-simple/date-simple.module.scss(1 hunks)src/shared/ui/date-simple/date-simple.test.tsx(1 hunks)src/shared/ui/date-simple/date-simple.tsx(1 hunks)src/shared/ui/date-simple/index.ts(1 hunks)src/shared/ui/date-start/index.ts(0 hunks)src/shared/ui/date-start/ui/date-start.tsx(0 hunks)src/shared/ui/short-info-panel/index.ts(1 hunks)src/shared/ui/short-info-panel/short-info-panel.module.scss(0 hunks)src/shared/ui/short-info-panel/short-info-panel.test.tsx(1 hunks)src/shared/ui/short-info-panel/short-info-panel.tsx(1 hunks)src/views/mentorship/ui/mentorship-courses/mentorship-courses.tsx(1 hunks)src/widgets/hero-course/ui/hero-course.tsx(2 hunks)
π€ Files with no reviewable changes (5)
- src/shared/ui/date-lang/index.ts
- src/shared/ui/date-start/index.ts
- src/shared/ui/short-info-panel/short-info-panel.module.scss
- src/shared/ui/date-lang/date-lang.tsx
- src/shared/ui/date-start/ui/date-start.tsx
β Files skipped from review due to trivial changes (2)
- src/shared/ui/short-info-panel/index.ts
- src/shared/ui/date-simple/index.ts
π§° Additional context used
πͺ Biome (1.9.4)
src/shared/ui/date-simple/date-simple.test.tsx
[error] 60-62: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragment
(lint/complexity/noUselessFragments)
β° Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: CI
π Additional comments (12)
src/entities/course/ui/course-item/course-item.tsx (1)
36-42: LGTM! Clean implementation of ShortInfoPanel.The component maintains good accessibility with proper ARIA attributes and follows React best practices.
src/entities/course/ui/course-card/course-card.test.tsx (1)
23-24: Add test cases for mentoring dates.While the new properties are added to mockProps, there are no test cases verifying the rendering of mentoring dates when they are present.
Consider adding test cases for:
- When both mentoring dates are present
- When only start date is present
- When only end date is present
src/shared/ui/short-info-panel/short-info-panel.tsx (1)
14-22: LGTM! Well-structured props interface.The props interface is clear and properly typed.
src/widgets/hero-course/ui/hero-course.tsx (1)
57-63: LGTM! Clean migration to ShortInfoPanel.The replacement of DateLang with ShortInfoPanel is well-implemented with all necessary props.
src/shared/ui/date-simple/date-simple.test.tsx (1)
1-69: LGTM! Comprehensive test coverage.The test suite thoroughly covers all essential scenarios for the DateSimple component.
π§° Tools
πͺ Biome (1.9.4)
[error] 60-62: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragment(lint/complexity/noUselessFragments)
src/entities/course/ui/course-card/course-card.tsx (2)
26-30: LGTM! Well-structured type definition.The CourseCardProps type is properly extended with new optional properties.
60-87: LGTM! Clean conditional rendering implementation.The component elegantly handles both mentoring and non-mentoring display modes.
src/shared/helpers/getActualData.test.ts (1)
19-122: LGTM! Thorough test data setup.The mock data correctly includes the new mentoring date properties.
dev-data/courses.data.ts (1)
43-44: LGTM! Consistent data model extension.The new mentoring date properties are consistently added across all course objects.
Also applies to: 65-66, 87-88, 108-109, 129-130, 150-151, 171-172, 193-194, 214-215
src/shared/__tests__/constants.ts (1)
47-48: LGTM! Consistent addition of mentoring date properties.The new properties
personalMentoringStartDateandpersonalMentoringEndDateare consistently added across all course objects with appropriate null initialization.Also applies to: 69-70, 91-92, 112-113, 133-134, 154-155, 175-176, 196-197
src/shared/ui/date-simple/date-simple.module.scss (1)
1-16: LGTM! Clean and consistent styling.The styles follow best practices with proper use of flexbox, consistent spacing, and design system variables.
src/entities/course/ui/course-item/course-item.module.scss (1)
22-30: LGTM! Improved layout structure.The update to use flexbox with consistent gap spacing improves maintainability and follows modern CSS best practices.
| clonedCourse.personalMentoringStartDate = dayJS( | ||
| currApiCourse.personalMentoringStartDate, | ||
| ).format(COURSE_DATE_FORMAT); | ||
| clonedCourse.personalMentoringEndDate = dayJS(currApiCourse.personalMentoringEndDate).format( | ||
| COURSE_DATE_FORMAT, | ||
| ); |
There was a problem hiding this comment.
Add validation for mentoring dates.
The current implementation might format invalid dates if the API returns null or undefined values.
- clonedCourse.personalMentoringStartDate = dayJS(
- currApiCourse.personalMentoringStartDate,
- ).format(COURSE_DATE_FORMAT);
- clonedCourse.personalMentoringEndDate = dayJS(currApiCourse.personalMentoringEndDate).format(
- COURSE_DATE_FORMAT,
- );
+ clonedCourse.personalMentoringStartDate = currApiCourse.personalMentoringStartDate
+ ? dayJS(currApiCourse.personalMentoringStartDate).format(COURSE_DATE_FORMAT)
+ : null;
+ clonedCourse.personalMentoringEndDate = currApiCourse.personalMentoringEndDate
+ ? dayJS(currApiCourse.personalMentoringEndDate).format(COURSE_DATE_FORMAT)
+ : null;π 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.
| clonedCourse.personalMentoringStartDate = dayJS( | |
| currApiCourse.personalMentoringStartDate, | |
| ).format(COURSE_DATE_FORMAT); | |
| clonedCourse.personalMentoringEndDate = dayJS(currApiCourse.personalMentoringEndDate).format( | |
| COURSE_DATE_FORMAT, | |
| ); | |
| clonedCourse.personalMentoringStartDate = currApiCourse.personalMentoringStartDate | |
| ? dayJS(currApiCourse.personalMentoringStartDate).format(COURSE_DATE_FORMAT) | |
| : null; | |
| clonedCourse.personalMentoringEndDate = currApiCourse.personalMentoringEndDate | |
| ? dayJS(currApiCourse.personalMentoringEndDate).format(COURSE_DATE_FORMAT) | |
| : null; |
|
Lighthouse Report:
|
There was a problem hiding this comment.
Actionable comments posted: 0
π§Ή Nitpick comments (3)
src/shared/ui/short-info-panel/short-info-panel.test.tsx (1)
42-59: Consider adding tests for edge cases.While the current test coverage is good, consider adding tests for:
- Empty or invalid dates
- Error states
- Different language codes
- Various combinations of optional props
Would you like me to help generate additional test cases for these scenarios?
src/shared/ui/date-simple/date-simple.test.tsx (2)
11-11: Consider adding edge cases and using more realistic test data.
- The test dates use year 2088, which is far in the future. Consider using more realistic dates.
- Consider adding test cases for:
- Invalid date formats
- Empty strings
- Null values
- End date before start date
Also applies to: 21-21, 41-41, 55-55
58-64: Remove unnecessary Fragment wrapper.The Fragment is redundant as it wraps a single child element.
render( <DateSimple startDate={startDate}> - <> <span>{additionalInfoText}</span> - </> </DateSimple>, );π§° Tools
πͺ Biome (1.9.4)
[error] 60-62: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragment(lint/complexity/noUselessFragments)
π Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
π Files selected for processing (7)
src/entities/course/ui/course-item/course-item.test.tsx(1 hunks)src/shared/helpers/getActualData.test.ts(3 hunks)src/shared/helpers/getCourseDate.ts(1 hunks)src/shared/ui/date-simple/date-simple.test.tsx(1 hunks)src/shared/ui/short-info-panel/index.ts(1 hunks)src/shared/ui/short-info-panel/short-info-panel.test.tsx(2 hunks)src/shared/ui/short-info-panel/short-info-panel.tsx(1 hunks)
π§ Files skipped from review as they are similar to previous changes (5)
- src/shared/ui/short-info-panel/index.ts
- src/entities/course/ui/course-item/course-item.test.tsx
- src/shared/helpers/getCourseDate.ts
- src/shared/ui/short-info-panel/short-info-panel.tsx
- src/shared/helpers/getActualData.test.ts
π§° Additional context used
πͺ Biome (1.9.4)
src/shared/ui/date-simple/date-simple.test.tsx
[error] 60-62: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragment
(lint/complexity/noUselessFragments)
π Additional comments (4)
src/shared/ui/short-info-panel/short-info-panel.test.tsx (2)
1-9: LGTM! Import statements are well-organized.The imports are logically grouped and include all necessary dependencies for the test suite.
10-40: LGTM! Test cases are well-structured and focused.Each test case verifies a specific aspect of the component with clear assertions.
src/shared/ui/date-simple/date-simple.test.tsx (2)
1-8: LGTM! Well-organized imports.All necessary testing libraries and dependencies are properly imported.
9-69: LGTM! Comprehensive test coverage.The test cases effectively cover the main component scenarios including basic rendering, full props, TBD cases, and children rendering.
π§° Tools
πͺ Biome (1.9.4)
[error] 60-62: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragment(lint/complexity/noUselessFragments)
|
Lighthouse Report:
|
|
Lighthouse Report:
|
There was a problem hiding this comment.
Actionable comments posted: 2
π§Ή Nitpick comments (1)
src/shared/ui/short-info-panel/short-info-panel.test.tsx (1)
10-67: Add test coverage for missing scenarios.Consider adding tests for:
- The
withMarginprop to verify margin styling- Null values for
startDateandregistrationEndDate
π Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
π Files selected for processing (9)
src/entities/course/constants.ts(1 hunks)src/entities/course/helpers/sync-with-api-data.ts(2 hunks)src/entities/course/ui/course-card/course-card.tsx(4 hunks)src/entities/course/ui/course-item/course-item.tsx(2 hunks)src/shared/ui/date-simple/date-simple.test.tsx(1 hunks)src/shared/ui/date-simple/date-simple.tsx(1 hunks)src/shared/ui/short-info-panel/short-info-panel.test.tsx(2 hunks)src/shared/ui/short-info-panel/short-info-panel.tsx(1 hunks)src/widgets/hero-course/ui/hero-course.tsx(2 hunks)
β Files skipped from review due to trivial changes (1)
- src/entities/course/constants.ts
π§ Files skipped from review as they are similar to previous changes (5)
- src/entities/course/helpers/sync-with-api-data.ts
- src/entities/course/ui/course-card/course-card.tsx
- src/widgets/hero-course/ui/hero-course.tsx
- src/entities/course/ui/course-item/course-item.tsx
- src/shared/ui/date-simple/date-simple.tsx
π§° Additional context used
πͺ Biome (1.9.4)
src/shared/ui/date-simple/date-simple.test.tsx
[error] 99-101: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragment
(lint/complexity/noUselessFragments)
π Additional comments (1)
src/shared/ui/short-info-panel/short-info-panel.tsx (1)
14-22: Consider making the label prop optional without default value.As suggested in the previous review, we could remove the default label value and update the type to
label?: string.
| startDate={ | ||
| startDate && registrationEndDate | ||
| ? calculateFreshDate(startDate, registrationEndDate) | ||
| : null | ||
| } |
There was a problem hiding this comment.
π οΈ Refactor suggestion
Add null check for calculateFreshDate arguments.
The null check should be moved inside the ternary to prevent potential issues:
-startDate && registrationEndDate
- ? calculateFreshDate(startDate, registrationEndDate)
- : null
+startDate && registrationEndDate && calculateFreshDate(startDate, registrationEndDate) || nullπ 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.
| startDate={ | |
| startDate && registrationEndDate | |
| ? calculateFreshDate(startDate, registrationEndDate) | |
| : null | |
| } | |
| startDate={ | |
| startDate && registrationEndDate && calculateFreshDate(startDate, registrationEndDate) || null | |
| } |
| <DateSimple startDate={startDate}> | ||
| <> | ||
| <span>{additionalInfoText}</span> | ||
| </> | ||
| </DateSimple>, | ||
| ); |
There was a problem hiding this comment.
π οΈ Refactor suggestion
Remove unnecessary Fragment wrapper.
The Fragment wrapper is redundant as it contains only one child element.
render(
<DateSimple startDate={startDate}>
- <>
<span>{additionalInfoText}</span>
- </>
</DateSimple>,
);π 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.
| <DateSimple startDate={startDate}> | |
| <> | |
| <span>{additionalInfoText}</span> | |
| </> | |
| </DateSimple>, | |
| ); | |
| <DateSimple startDate={startDate}> | |
| <span>{additionalInfoText}</span> | |
| </DateSimple>, |
π§° Tools
πͺ Biome (1.9.4)
[error] 99-101: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragment
(lint/complexity/noUselessFragments)
| endDate={personalMentoringEndDate} | ||
| labelSeparator={LABELS.MENTORING_DATES_SEPARATOR} | ||
| startDate={personalMentoringStartDate || TO_BE_DETERMINED} | ||
| endDate={personalMentoringStartDate ? personalMentoringEndDate : null} |
There was a problem hiding this comment.
Should it be endDate={personalMentoringEndDate ? personalMentoringEndDate : null}?
What type of PR is this? (select all that apply)
Description
"Mentor activities" Start Date - End Date.
Related Tickets & Documents
Screenshots, Recordings
Desktop
Mobile
Tablet (from 810 px to 1080) dates look like below, not an ideal, but fitting. Rework is suggested as a minor issue in the separate ticket:
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
Summary by CodeRabbit
New Features
DateSimplecomponent for improved date display functionality.ShortInfoPanelcomponent to replace legacy date display components.CourseCardandHeroCoursecomponents to utilize new mentoring display features.Style
Tests & Refactor