-
-
Notifications
You must be signed in to change notification settings - Fork 10
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. Weβll occasionally send you account related emails.
Already on GitHub? Sign in to your account
614-fix: Pre school subtitle #675
Conversation
I think we changed it on purpose (in the other direction) - somewhere in other files. @SpaNb4 do you remember this discussion? |
Yeah, you're right #576 (comment) |
Lighthouse Report:
|
Lighthouse Report:
|
π Walkthroughπ WalkthroughWalkthroughThe pull request introduces several structural changes to course-related data, including the addition of a Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
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
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
π§Ή Outside diff range and nitpick comments (1)
dev-data/courses.data.ts (1)
27-27
: Consider using a constant for the subtitleInstead of hardcoding the subtitle value, consider adding it to the
COURSE_TITLES
constant to maintain consistency and make future updates easier.- subTitle: 'Pre-school RU', + subTitle: COURSE_TITLES.JS_PRESCHOOL_RU_SUBTITLE,
π Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
π Files selected for processing (13)
dev-data/about-course.data.tsx
(1 hunks)dev-data/courses.data.ts
(9 hunks)src/app/courses/javascript-preschool-ru/page.tsx
(1 hunks)src/entities/course/types.ts
(1 hunks)src/shared/__tests__/constants.ts
(6 hunks)src/shared/helpers/getActualData.test.ts
(3 hunks)src/views/javascript-preschool-ru.tsx
(1 hunks)src/views/javascript-ru.tsx
(1 hunks)src/widgets/about-course/ui/about-course/about-course.test.tsx
(2 hunks)src/widgets/about-course/ui/about-course/about-course.tsx
(1 hunks)src/widgets/hero-course/ui/hero-course.test.tsx
(1 hunks)src/widgets/hero-course/ui/hero-course.tsx
(2 hunks)src/widgets/training-program/ui/training-program.test.tsx
(2 hunks)
π Additional comments (15)
src/entities/course/types.ts (1)
28-28
: LGTM! Verify subtitle values in course data.
The addition of subTitle
property is a good approach. Being nullable allows flexibility for courses without subtitles.
β Verification successful
LGTM! Subtitle implementation is consistent across the codebase
The subTitle
property is properly used in course data files and components. Found evidence of:
- Correct type definition in
src/entities/course/types.ts
- Consistent usage in course data (
dev-data/courses.data.ts
) - Proper UI implementation in components (
hero-course
,communication
, etc.) - Appropriate handling of null values
π Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify subtitle values in course data files
rg -l "subTitle.*pre-school"
Length of output: 30
Script:
#!/bin/bash
# Search for any usage of subTitle in data files
rg -l "subTitle"
# Search for course data files
fd "course.*\.(json|ts|js|tsx|jsx)"
# Check for any existing subtitle or sub-title patterns in course data
rg -i "sub.?title"
Length of output: 15704
src/views/javascript-preschool-ru.tsx (2)
19-19
: LGTM! Props simplified.
Removal of the type
prop simplifies the component interface, aligning with the new subTitle
property in Course type.
22-22
: LGTM! Props correctly updated.
HeroCourse and AboutCourse components now receive the correct props without the redundant type
prop.
Also applies to: 24-24
src/app/courses/javascript-preschool-ru/page.tsx (1)
16-16
: LGTM! Component usage updated.
Component props are correctly updated to match the new interface.
src/widgets/about-course/ui/about-course/about-course.tsx (1)
18-18
: LGTM: Simplified prop interface and content access
The removal of the type
prop in favor of using courseName
directly for content access is a clean improvement.
Let's verify all usages are updated:
Also applies to: 24-26, 30-30
β Verification successful
All usages of AboutCourse component have been updated correctly
All component invocations consistently use only course
and courseName
props, with no instances of the removed type
prop remaining. The type references found are just TypeScript type definitions, not prop usage.
π Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining usages of type prop with AboutCourse
rg "type.*AboutCourse"
Length of output: 458
Script:
#!/bin/bash
# Search for AboutCourse component usage to verify type prop removal
rg -A 2 "<AboutCourse"
# Check AboutCourseProps interface usage
ast-grep --pattern 'type AboutCourseProps = {
$$$
}'
Length of output: 4266
src/views/javascript-ru.tsx (1)
28-28
: LGTM: Consistent prop usage
The removal of the type
prop aligns with the AboutCourse component changes.
src/widgets/hero-course/ui/hero-course.tsx (2)
21-32
: LGTM: Clean prop interface update
Removing the type
prop and adding subTitle
to the destructured properties streamlines the component interface.
42-42
: Verify subtitle display for pre-school course
The implementation looks good, but let's verify it fixes issue #614.
β Verification successful
Subtitle implementation verified successfully
The implementation correctly displays the subtitle for pre-school courses. The component properly handles the subTitle
prop from the course data, and the test data confirms the presence of "Pre-school RU" subtitle in both test constants and course data.
π Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the pre-school course has the correct subtitle in the data
rg -A 1 "subTitle.*Pre-school RU"
Length of output: 304
Script:
#!/bin/bash
# Let's check the implementation of the hero component and how it handles the subtitle
ast-grep --pattern 'const HeroCourse = ({ subTitle, $$ }) => {
$$$
}'
Length of output: 73
Script:
#!/bin/bash
# Let's check the hero component implementation with a more flexible pattern
ast-grep --pattern 'const HeroCourse'
# Also check the file content directly
cat src/widgets/hero-course/ui/hero-course.tsx
Length of output: 4858
src/widgets/about-course/ui/about-course/about-course.test.tsx (1)
21-21
: LGTM! Changes align with PR objectives
The removal of the type
prop and addition of subTitle
property are consistent with the goal of fixing the pre-school subtitle issue.
Also applies to: 80-80
src/shared/helpers/getActualData.test.ts (1)
23-23
: LGTM! Type changes properly reflected in test data
The addition of the subTitle
property to all course mock objects maintains consistency with the updated Course type.
Also applies to: 54-54, 86-86
src/shared/__tests__/constants.ts (1)
28-28
: LGTM! Test data structure updated correctly
The addition of the subTitle
property across all mock courses maintains consistency with the production data structure.
Also applies to: 48-48, 68-68, 88-88, 108-108, 128-128
src/widgets/training-program/ui/training-program.test.tsx (1)
14-14
: LGTM! Test objects updated correctly
The addition of the subTitle
property to mock course objects maintains consistency with the production data structure.
Also applies to: 41-41
dev-data/courses.data.ts (1)
27-27
: LGTM! Production data structure updated correctly
The addition of the subTitle
property across all courses is consistent.
Also applies to: 47-47, 67-67, 87-87, 106-106, 125-125, 144-144, 164-164, 183-183
dev-data/about-course.data.tsx (2)
17-32
: LGTM: Well-structured localization constants
The new intro constants provide a clean separation of localized content with consistent structure.
35-42
: Verify course title mappings
The mappings look correct, but let's verify all course titles are properly mapped.
β Verification successful
Let me verify the COURSE_TITLES enum values to ensure we have complete coverage.
Course title mappings are correctly implemented
All course titles defined in COURSE_TITLES
(JS_PRESCHOOL_RU, JS_EN, JS_RU, REACT, ANGULAR, NODE, AWS_FUNDAMENTALS, AWS_CLOUD_DEVELOPER, AWS_DEVOPS) are properly mapped in the introLocalizedContent object with appropriate intro content (preSchoolIntro, enIntro, ruIntro).
π Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify all COURSE_TITLES are mapped in introLocalizedContent
# Find all COURSE_TITLES enum values
rg "COURSE_TITLES\." --type ts | grep -v "introLocalizedContent" > titles.txt
# Compare with mappings in introLocalizedContent
rg "\[COURSE_TITLES\." --type ts | grep "introLocalizedContent" > mappings.txt
# Show any titles that might be missing from mappings
diff titles.txt mappings.txt || true
rm titles.txt mappings.txt
Length of output: 7606
Script:
#!/bin/bash
# Find the COURSE_TITLES enum definition
ast-grep --pattern 'enum COURSE_TITLES {
$$$
}'
Length of output: 51
Script:
#!/bin/bash
# Let's try to find the COURSE_TITLES type definition using ripgrep
rg "type COURSE_TITLES|enum COURSE_TITLES|const COURSE_TITLES" -A 15
Length of output: 1144
Lighthouse Report:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
π§Ή Outside diff range and nitpick comments (1)
src/widgets/hero-course/ui/hero-course.test.tsx (1)
14-14
: Update test data to match actual use caseConsider using 'Pre-school RU' as the test subtitle to match the actual use case from the PR objectives.
- subTitle: 'Test Subtitle', + subTitle: 'Pre-school RU',
π Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
π Files selected for processing (1)
src/widgets/hero-course/ui/hero-course.test.tsx
(2 hunks)
π Additional comments (2)
src/widgets/hero-course/ui/hero-course.test.tsx (2)
32-57
: Well-structured test organization
Good job organizing tests into logical groups, improving readability and maintainability.
60-74
: Comprehensive subtitle test cases implemented
The test cases effectively verify subtitle rendering behavior for both positive and negative scenarios.
Lighthouse Report:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
π§Ή Outside diff range and nitpick comments (2)
src/widgets/hero-course/ui/hero-course.test.tsx (2)
14-29
: Consider using realistic test dataThe mock subtitle "Test Subtitle" could be replaced with "Pre-school RU" to better align with the actual use case from the PR objectives.
- subTitle: 'Test Subtitle', + subTitle: 'Pre-school RU',
Line range hint
30-50
: Reduce mock data duplicationConsider creating a base mock object and extending it for specific test cases to improve maintainability.
+const baseMockedCourse: Omit<Course, 'subTitle'> = { + id: '6', + title: COURSE_TITLES.NODE, + // ... other shared properties +}; -const mockedCourse: Course = { +const mockedCourse: Course = { + ...baseMockedCourse, subTitle: 'Pre-school RU', - // ... duplicated properties }; -const mockedCourseNoSubtitle: Course = { +const mockedCourseNoSubtitle: Course = { + ...baseMockedCourse, subTitle: null, - // ... duplicated properties };
π Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
π Files selected for processing (1)
src/widgets/hero-course/ui/hero-course.test.tsx
(2 hunks)
π Additional comments (2)
src/widgets/hero-course/ui/hero-course.test.tsx (2)
81-87
: LGTM!
Good use of queryByText
for checking non-existence of elements.
51-78
: Verify test isolation
Based on previous comments, there might be a test order dependency. Each test should be independent and not rely on the order of execution.
β Verification successful
After analyzing the test structure in the codebase, I can see that the tests in hero-course.test.tsx
follow a proper isolation pattern. The beforeEach
block only sets up the component render, and each test case independently verifies different aspects of the component without relying on state from other tests. The assertions are also independent and don't depend on the order of execution.
No test isolation issues found
The tests are properly isolated:
- Each test has its own assertions
- No shared mutable state between tests
beforeEach
only handles component setup- Tests verify independent aspects of the component
π Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for potential test dependencies
ast-grep --pattern 'beforeEach($$$)'
rg -A 5 'beforeEach|afterEach'
Length of output: 36685
Lighthouse Report:
|
What type of PR is this? (select all that apply)
Description
fix subtitle in hero widget pre-school cource
Related Tickets & Documents
Screenshots, Recordings
Please replace this line with any relevant images for UI changes.
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
Release Notes
New Features
subTitle
property to course objects, enhancing course detail visibility.Improvements
type
properties, focusing oncourseName
for localized content retrieval.These updates streamline user experience and improve content accessibility across the platform.