347-refactor: Fsd widget events #573
Conversation
LinderJK
requested review from
andron13,
dzmitry-varabei,
SpaNb4,
Quiddlee,
ansivgit and
natanchik
as code owners
📝 WalkthroughWalkthroughThe pull request introduces a new constant declaration in Changes
Assessment against linked issues
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
|
github-actions
bot
changed the title
![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 (2)
src/widgets/events/ui/events.tsx (2)
37-37: Image rendering looks good, minor suggestion.The changes align with the PR objectives. The Stub constant has been removed, and the Image component is now directly incorporated into the JSX with an improved alt text.
Consider extracting the condition
!actualEvents.lengthto a descriptive variable for improved readability:-{!actualEvents.length && (<Image src={photo3} alt="Speaker presenting at a event" className={cx('event-img')} />)} +{const noActualEvents = !actualEvents.length; +noActualEvents && (<Image src={photo3} alt="Speaker presenting at a event" className={cx('event-img')} />)}
39-40: Mapping function improvements look good.The changes align with the PR objectives. The mapping function now uses a more descriptive parameter name, and unnecessary type casting has been removed.
For consistency with modern JavaScript practices, consider using object property shorthand:
-{(nearestEvents).map((event) => ( - <EventCard key={event.title} {...event} date={dayjs(event.date).format('DD MMM YYYY')} /> -))} +{nearestEvents.map((event) => ( + <EventCard key={event.title} {...event} date={dayjs(event.date).format('DD MMM YYYY')} /> +))}
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
- src/widgets/events/constants.ts (1 hunks)
- src/widgets/events/ui/events.module.scss (1 hunks)
- src/widgets/events/ui/events.test.tsx (1 hunks)
- src/widgets/events/ui/events.tsx (2 hunks)
🔇 Additional comments (7)
src/widgets/events/constants.ts (3)
5-5: Constant placement approved.Matches PR objectives.
6-9: Constant implementation looks good.Clarify
staleAfterunit (days/hours)?
11-11: Nearest events constant approved.Correctly uses
displayedCardsQuantity.src/widgets/events/ui/events.module.scss (2)
8-8: Width increase for .info class.The width has been increased from 460px to 640px, allowing for more content.
Line range hint
1-46: Removed margin-top for .title and .subtitle.Margin-top properties for .title and .subtitle classes have been removed. This may affect vertical spacing.
src/widgets/events/ui/events.tsx (1)
4-4: Import changes look good.The modifications align with the PR objectives. Unused imports have been removed, and constants are now imported from a separate file, improving code organization.
Also applies to: 10-10
src/widgets/events/ui/events.test.tsx (1)
Line range hint
1-67: Rest of the file looks good.The existing tests cover various aspects of the Events component thoroughly. No further changes or improvements are necessary.
|
Lighthouse Report:
|
|
Lighthouse Report:
|
Co-authored-by: Bogdan <[email protected]>
|
Lighthouse Report:
|
|
Lighthouse Report:
|
|
Lighthouse Report:
|
|
run visual now |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
src/widgets/events/constants.ts (1)
6-6: Consider renaming for consistency.Rename
maxUpcomingEventsQuantitytomaxEventsCountfor consistency with other parts of the codebase.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (5)
- src/shared/constants.ts (1 hunks)
- src/widgets/events/constants.ts (1 hunks)
- src/widgets/events/ui/events.module.scss (1 hunks)
- src/widgets/events/ui/events.test.tsx (1 hunks)
- src/widgets/events/ui/events.tsx (2 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/shared/constants.ts
🔇 Additional comments (14)
src/widgets/events/constants.ts (3)
1-4: Imports look good.The necessary dependencies are correctly imported.
7-12: Constants implementation looks good.The
actualEventsandnearestEventsare correctly implemented as per the PR objectives.
14-14: rsLifetime calculation looks good.The constant correctly uses RS_FOUNDATION_YEAR from shared constants, addressing previous concerns about hardcoding.
src/widgets/events/ui/events.module.scss (4)
1-4: Improved class structure.Simplified class naming and direct application of flexbox properties.
6-11: Width adjustment as discussed.Width set to 460px with responsive adjustment, aligning with previous review discussion.
14-34: Review.event-imgwidth.Consider if 500px width for
.event-imgis appropriate for all viewports between mobile landscape and full width.
37-40: Enhanced responsiveness for medium laptops.New media query improves layout adaptation for medium laptop screens.
src/widgets/events/ui/events.test.tsx (1)
Line range hint
1-69: Test cases appropriately maintained.The existing test cases have been preserved, which is correct as the PR doesn't introduce new functionality requiring additional tests.
src/widgets/events/ui/events.tsx (6)
3-3: Import cleanup approved.Removal of unused
Eventimport aligns with PR objectives.
9-9: Constants import approved.Import of constants from a separate file improves code organization.
17-18: Class name updates approved.Simplified article class and updated div classes improve consistency.
32-32: Text update approved.Updated text provides more specific information about the events.
37-39: Image rendering update approved.Direct use of
Imagecomponent and improvedalttext enhance code clarity and accessibility.
41-46: Event mapping update approved.Use of
eventinstead ofiand simplifiedEventCardrendering improve code readability.
What type of PR is this? (select all that apply)
Description
@widgets/eventsevents.test.tsxto the ui folder.displayedCardsQuantity,actualEvents,nearestEvents, andrsLifetimetoconstants.ts.Stubconstant and added Image to JSX.Event[].map(i)tomap(event).RS_FOUNDATION_YEARRelated Tickets & Documents
Screenshots, Recordings
No 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
Summary by CodeRabbit