-
Notifications
You must be signed in to change notification settings - Fork 8
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
574-refactor: List component #575
base: main
Are you sure you want to change the base?
Conversation
Lighthouse Report:
|
π WalkthroughWalkthroughThe changes involve a significant refactor of the 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: 2
π§Ή Outside diff range and nitpick comments (8)
src/shared/types.ts (2)
3-3
:ListData
type definition is flexible and well-structured.The type allows for various list data scenarios. Consider using
Array<string | LinkList>
instead of(string | LinkList)[]
for consistency with TypeScript best practices.
5-5
:ListType
definition aligns with PR objectives.The union type effectively replaces the boolean
marked
prop. Consider using an enum if you anticipate adding more list types in the future.src/widgets/study-path/ui/stage-card/stage-card.types.ts (1)
19-19
: StageCardProps update is on point.The 'type' property replacement enhances flexibility. Consider adding a JSDoc comment to explain its usage.
src/shared/ui/list/list.module.scss (1)
12-19
: New.compact
class looks good.The
.compact
class effectively reduces spacing and adjusts font size for larger screens. This aligns with the PR objectives.Consider adding a comment explaining the purpose of the
.compact
class for better maintainability.src/widgets/study-path/ui/stages/stages.tsx (1)
8-8
: StagesProps interface updated.The
type
prop replacement enhances component flexibility. Consider adding a JSDoc comment to explain the purpose and possible values of thetype
prop.src/widgets/study-path/ui/stage-card/stage-card.tsx (1)
30-30
: List component update approved.The
type
prop is correctly passed to theList
component. Consider adding a comment explaining the possible values oftype
for better clarity.- {list && <List data={list} type={type} />} + {list && <List data={list} type={type} />} {/* type can be 'marked' or 'unmarked' */}src/widgets/study-path/ui/study-path.tsx (1)
15-15
: StudyPathProps interface updated.The
marked
prop has been replaced withtype
, aligning with PR objectives.Consider adding a JSDoc comment to describe the
type
prop and its possible values.src/shared/ui/list/list.tsx (1)
22-24
: Simplify the 'unmarked' variant inlistVariants
In the
'unmarked'
variant, the class is set tocx('')
, which returns an empty string. This might be unnecessary. You can omit the class assignment for'unmarked'
to simplify the code.Consider changing it to:
type: { marked: cx('marked'), - unmarked: cx(''), + unmarked: undefined, },
π Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
π Files selected for processing (12)
- dev-data/courses-data.types.ts (1 hunks)
- dev-data/training-program.data.tsx (3 hunks)
- src/pages/javascript-en.tsx (1 hunks)
- src/shared/types.ts (1 hunks)
- src/shared/ui/list/index.ts (1 hunks)
- src/shared/ui/list/list.module.scss (1 hunks)
- src/shared/ui/list/list.test.tsx (1 hunks)
- src/shared/ui/list/list.tsx (1 hunks)
- src/widgets/study-path/ui/stage-card/stage-card.tsx (2 hunks)
- src/widgets/study-path/ui/stage-card/stage-card.types.ts (2 hunks)
- src/widgets/study-path/ui/stages/stages.tsx (2 hunks)
- src/widgets/study-path/ui/study-path.tsx (4 hunks)
π Additional comments (18)
src/shared/ui/list/index.ts (1)
1-1
: Export simplification approved.The removal of type exports aligns with the refactoring goals.
Verify type relocation:
src/shared/types.ts (1)
1-1
: Import statement looks good.The import of
LinkList
is necessary for theListData
type definition.src/widgets/study-path/ui/stage-card/stage-card.types.ts (1)
1-1
: Import update looks good.The addition of ListType aligns with the refactoring goals.
dev-data/courses-data.types.ts (1)
2-2
: Import statement updated.The
ListData
type is now imported from@/shared/types
instead of@/shared/ui/list
. This change aligns with the PR objective of moving type definitions out of the component.β Verification successful
Adjusting the search command for TypeScript files.
The previous
rg
command failed because'typescript'
is not a recognized file type. Let's retry the search by specifying the file extensions.ts
and.tsx
.--- ```markdown
Updating the search command for TypeScript files.
The previous
rg
command had an invalid type definition. Let's retry by specifying the file extensions using-g
flags.
<!-- [monologue] [task finalize_verification_1/1] Analyzed the output of the latest rg command. [observations] - Found two files where `ListData` is imported from `'@/shared/types'`. - These imports are consistent with the recent change. [analysis] The import statement has been successfully updated in the identified files, aligning with the PR's objective. [actions] - Conclude the verification process as successful. -->Import statement consistently updated across the codebase.
The
ListData
type is now correctly imported from@/shared/types
in all relevant files.π Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify consistent usage of the new import across the codebase rg --type typescript "import.*ListData.*from '@/shared/types'" -g '!dev-data/courses-data.types.ts'Length of output: 142
Script:
#!/bin/bash # Verify consistent usage of the new import across the codebase by searching .ts and .tsx files rg "import.*ListData.*from '@/shared/types'" --type-add 'ts=**/*.ts' --type-add 'tsx=**/*.tsx' --type ts --type tsx -g '!dev-data/courses-data.types.ts'Length of output: 220
Script:
#!/bin/bash # Verify consistent usage of the new import across .ts and .tsx files rg "import.*ListData.*from '@/shared/types'" -g "*.ts" -g "*.tsx" -g '!dev-data/courses-data.types.ts'Length of output: 296
src/widgets/study-path/ui/stages/stages.tsx (3)
2-2
: Import added for ListType.The new import is correctly added and uses the project's import alias convention.
11-11
: Stages component signature updated.The component signature correctly reflects the
StagesProps
interface change.
29-29
: Prop passed to StageCard updated.The
type
prop is correctly passed toStageCard
. EnsureStageCard
component is updated to handle this new prop.src/widgets/study-path/ui/stage-card/stage-card.tsx (1)
20-20
: Prop update approved.The addition of the
type
prop aligns with the PR objectives.src/shared/ui/list/list.test.tsx (1)
30-31
: Test case updated to reflect new API.The test now checks for 'unmarked' type instead of false 'marked' prop. This aligns with the component's refactored API.
src/pages/javascript-en.tsx (1)
25-25
: Prop change looks good.The
marked
prop has been correctly updated totype="marked"
for theStudyPath
component.src/widgets/study-path/ui/study-path.tsx (2)
4-4
: Import added for ListType.The import is correctly placed and aligns with the PR objectives.
34-34
: StudyPath component updated to usetype
prop.The component signature and
Stages
prop have been correctly updated.Verify that the
Stages
component correctly handles the newtype
prop:Also applies to: 51-51
dev-data/training-program.data.tsx (6)
204-204
: LGTM: Prop change aligns with refactor.The
type="unmarked"
prop replaces the previousmarked
boolean, matching the PR's refactor objectives.
215-215
: LGTM: Consistent prop change.The
type="unmarked"
prop is consistently applied here, maintaining the refactor's uniformity.
227-227
: LGTM: Prop change maintains consistency.The
type="unmarked"
prop is applied here as well, ensuring consistency across the component usage.
232-232
: LGTM: Prop change follows pattern.The
type="unmarked"
prop is correctly applied, maintaining the refactor's consistency.
238-238
: LGTM: Refactor successfully applied.The
type="unmarked"
prop is consistently used here and throughout the file, completing the List component refactor for this course content.
Line range hint
204-238
: Summary: Successful List component refactor.The changes consistently replace the
marked
boolean prop withtype="unmarked"
throughout the 'js / front-end pre-school ru' course content. This refactor enhances the List component's flexibility and aligns with the PR's objectives.
type ListProps = Pick<HTMLAttributes<HTMLElement>, 'className'> | ||
& VariantProps<typeof listVariants> | ||
& Record<'data', ListData>; |
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.
π οΈ Refactor suggestion
Consider extending additional HTML attributes for flexibility
Currently, ListProps
only picks className
from HTMLAttributes<HTMLElement>
. Extending other HTML attributes (e.g., id
, style
, onClick
) could enhance the component's usability.
You could adjust the type definition as follows:
-type ListProps = Pick<HTMLAttributes<HTMLElement>, 'className'>
+type ListProps = HTMLAttributes<HTMLElement>
& VariantProps<typeof listVariants>
& { data: ListData };
π 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.
type ListProps = Pick<HTMLAttributes<HTMLElement>, 'className'> | |
& VariantProps<typeof listVariants> | |
& Record<'data', ListData>; | |
type ListProps = HTMLAttributes<HTMLElement> | |
& VariantProps<typeof listVariants> | |
& { data: ListData }; |
})} | ||
data-testid="list" | ||
> | ||
{data.map((listItem) => { |
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.
Ensure consistent structure for listItem
when generating keys
The keyListItem
is derived using listItem[0].id
when listItem
is not a string. This assumes that listItem
is an array with the first element having an id
property. To avoid potential runtime errors, ensure that this structure is consistent or adjust the key generation logic.
Consider verifying the structure of listItem
or updating the key extraction:
-const keyListItem = isLink ? listItem[0].id : listItem;
+const keyListItem = isLink ? listItem.id : listItem;
Ensure that listItem
has an id
property when it's not a string.
Committable suggestion was skipped due to low confidence.
@@ -0,0 +1,5 @@ | |||
import { LinkList } from '@/widgets/required/required.types'; | |||
|
|||
export type ListData = (string | LinkList)[] | [] | undefined; |
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.
Do we really ned undefined
here?
export type ListData = (string | LinkList)[] | [] | undefined; | ||
type ListProps = Pick<HTMLAttributes<HTMLElement>, 'className'> | ||
& VariantProps<typeof listVariants> | ||
& Record<'data', ListData>; |
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.
I'd use { data: ListData };
instead of Record<'data', ListData>
to make it more clear
size: { | ||
compact: cx('compact'), | ||
medium: cx('medium'), | ||
}, |
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.
Do we really need that prop? I can't find any usage of compact
What type of PR is this? (select all that apply)
Description
shared/types.ts
,cva
forList
components' variants,marked
to proptype
, which can be 'marked', 'unmarked' or smth else,compact
which can be used inAboutCourse
componentRelated Tickets & Documents
Added/updated tests?
Summary by CodeRabbit
New Features
type
prop for various components, replacing the previousmarked
prop for better clarity and functionality..compact
class to enhance list item styling options.ListData
andListType
to improve data handling.Bug Fixes
List
component.Refactor
List
,StageCard
,Stages
, andStudyPath
.