774-feat: Mentorship post release updates: Course card design update #781
Conversation
There was a problem hiding this comment.
Copilot reviewed 8 out of 18 changed files in this pull request and generated no comments.
Files not reviewed (10)
- src/core/styles/_constants.scss: Language not supported
- src/core/styles/_mixins.scss: Language not supported
- src/entities/course/ui/course-card/course-card.module.scss: Language not supported
- src/shared/ui/date-lang/date-lang.module.scss: Language not supported
- src/shared/ui/link-custom/link-custom.module.scss: Language not supported
- src/views/mentorship/ui/mentorship-courses/mentorship-courses.module.scss: Language not supported
- src/widgets/courses/ui/courses.module.scss: Language not supported
- src/entities/course/ui/course-card/course-card.test.tsx: Evaluated as low risk
- src/shared/icons/arrow-icon.tsx: Evaluated as low risk
- src/shared/ui/date-lang/date-lang.test.tsx: Evaluated as low risk
Quiddlee
changed the title
Quiddlee
changed the title
|
Hmm, the cards feel a bit too big now. I'm wondering if that'll hurt the UX a bit? current look: |
SpaNb4
requested review from
andron13,
dzmitry-varabei,
ansivgit and
natanchik
as code owners
📝 WalkthroughWalkthroughThis pull request applies numerous styling adjustments and minor refactoring across several SCSS and TSX files without changing component functionality. It reorders and removes redundant CSS declarations, adds new easing function variables and mixin updates, refines component props (e.g., removal of “mode” and addition of “size”), and updates tests accordingly. Several UI components such as CourseCard, DateLang, LinkCustom, and ArrowIcon are updated to simplify their internal logic and rendering. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant CC as CourseCard
participant DL as DateLang
participant LC as LinkCustom
participant AI as ArrowIcon
U->>CC: Render CourseCard (with size prop)
CC->>CC: Compute classes & dateText based on size
CC->>DL: Render DateLang (with dateText and language)
CC->>LC: Render LinkCustom (variant: secondary)
LC->>AI: Render ArrowIcon (default sizing)
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: 2
🔭 Outside diff range comments (1)
src/shared/icons/arrow-icon.tsx (1)
5-13: 🛠️ Refactor suggestionConsider specifying default dimensions.
Removing the size prop without specifying default dimensions could lead to inconsistent icon sizes across different contexts.
export const ArrowIcon = () => { return ( <Image + width={24} + height={24} src={arrow} alt="arrow icon" data-testid="arrow-icon" /> ); };
🧹 Nitpick comments (7)
src/widgets/courses/ui/courses.tsx (1)
28-28: Consider documenting size prop impact.The addition of
size="sm"addresses card size concerns raised in PR comments. Consider documenting the exact dimensions for each size variant ("sm", "md") to help track UI consistency.src/shared/ui/link-custom/link-custom.tsx (1)
80-82: Simplify DOM structure.The additional
spanwrapper is unnecessary unless specific styling requirements exist.- <span className={cx('icon-wrapper')}> - {!disabled && resolveIcon()} - </span> + {!disabled && resolveIcon()}src/views/mentorship/ui/mentorship-courses/mentorship-courses.module.scss (1)
14-16: Review layout breakpoint change.Changing to
media-tablet-largemeans the layout will maintain 2 columns longer, which might address the card size concerns mentioned in PR feedback. However, verify that this doesn't cause layout issues on tablet-sized devices.Consider implementing a more flexible grid system using
minmaxandauto-fit:- grid-template-columns: repeat(2, 1fr); + grid-template-columns: repeat(auto-fit, minmax(300px, 1fr));src/shared/ui/date-lang/date-lang.module.scss (2)
14-15: Consider maintaining previous gap size.The increased gap (8px) contributes to larger overall component size. Consider keeping the previous 4px gap to maintain compact design.
- gap: 8px; + gap: 4px;
28-31: Review icon size increase.Larger icons (24px) may impact overall card size. Consider if 16px icons would suffice for visual hierarchy.
- width: 24px; - height: 24px; + width: 16px; + height: 16px;src/core/styles/_mixins.scss (1)
54-56: Consider the impact of increased card dimensions.The card dimensions have been significantly increased (width: 310px, height: 300px). This aligns with user feedback expressing concerns about larger card sizes potentially impacting UX.
Consider A/B testing these dimensions to validate the impact on user experience across different viewport sizes.
src/entities/course/ui/course-card/course-card.tsx (1)
41-47: Consider extracting size-based logic.The size-based text and font size logic could be moved to a separate utility function for better maintainability.
+const getCardConfig = (size: 'sm' | 'md') => ({ + dateText: size === 'sm' ? 'Start:' : null, + fontSize: size === 'md' ? 'large' : 'small' +}); + export const CourseCard = ({ // ...props }) => { - const dateText = size === 'sm' ? 'Start:' : null; - const fontSize = size === 'md' ? 'large' : 'small'; + const { dateText, fontSize } = getCardConfig(size);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
src/shared/assets/icons/mic.svgis excluded by!**/*.svgsrc/shared/assets/icons/note-icon.svgis excluded by!**/*.svgsrc/shared/assets/svg/arrow.svgis excluded by!**/*.svg
📒 Files selected for processing (26)
src/app/docs/components/docs-menu/docs-menu.module.scss(1 hunks)src/core/base-layout/components/header/burger/burger.module.scss(1 hunks)src/core/base-layout/components/header/header.module.scss(1 hunks)src/core/styles/_constants.scss(1 hunks)src/core/styles/_mixins.scss(1 hunks)src/core/styles/index.scss(1 hunks)src/entities/course/ui/course-card/course-card.module.scss(2 hunks)src/entities/course/ui/course-card/course-card.test.tsx(1 hunks)src/entities/course/ui/course-card/course-card.tsx(1 hunks)src/entities/mentor/ui/mentor-feedback-card/mentor-feedback-card.module.scss(2 hunks)src/shared/icons/arrow-icon.tsx(1 hunks)src/shared/ui/date-lang/date-lang.module.scss(1 hunks)src/shared/ui/date-lang/date-lang.test.tsx(1 hunks)src/shared/ui/date-lang/date-lang.tsx(2 hunks)src/shared/ui/link-custom/link-custom.module.scss(3 hunks)src/shared/ui/link-custom/link-custom.tsx(2 hunks)src/shared/ui/modal/modal.module.scss(2 hunks)src/shared/ui/video-playlist-with-player/playlist/playlist.module.scss(1 hunks)src/views/mentorship/mentorship-hero/ui/mentorship-hero.module.scss(1 hunks)src/views/mentorship/ui/mentor-activities/ui/activity-card/activity-card.module.scss(2 hunks)src/views/mentorship/ui/mentor-activities/ui/activity-card/activity-card.test.tsx(1 hunks)src/views/mentorship/ui/mentorship-courses/mentorship-courses.module.scss(1 hunks)src/widgets/about-video/ui/about-video.module.scss(0 hunks)src/widgets/courses/ui/courses.module.scss(1 hunks)src/widgets/courses/ui/courses.tsx(1 hunks)src/widgets/hero-course/ui/hero-course.tsx(0 hunks)
💤 Files with no reviewable changes (2)
- src/widgets/about-video/ui/about-video.module.scss
- src/widgets/hero-course/ui/hero-course.tsx
✅ Files skipped from review due to trivial changes (10)
- src/widgets/courses/ui/courses.module.scss
- src/app/docs/components/docs-menu/docs-menu.module.scss
- src/shared/ui/video-playlist-with-player/playlist/playlist.module.scss
- src/core/base-layout/components/header/header.module.scss
- src/core/base-layout/components/header/burger/burger.module.scss
- src/core/styles/index.scss
- src/entities/mentor/ui/mentor-feedback-card/mentor-feedback-card.module.scss
- src/views/mentorship/mentorship-hero/ui/mentorship-hero.module.scss
- src/shared/ui/modal/modal.module.scss
- src/views/mentorship/ui/mentor-activities/ui/activity-card/activity-card.module.scss
🔇 Additional comments (11)
src/shared/ui/date-lang/date-lang.test.tsx (1)
25-25: Test update looks good.The removal of the mode prop from test cases maintains test coverage while aligning with component changes.
src/views/mentorship/ui/mentor-activities/ui/activity-card/activity-card.test.tsx (1)
51-51: Good test improvement.Using
toHaveTextContentinstead of checkinginnerHTMLis a more robust approach for testing text content.src/core/styles/_constants.scss (1)
86-88: LGTM! Good choice of easing functions.The added easing functions follow Material Design standards, providing smooth and natural animations.
src/entities/course/ui/course-card/course-card.test.tsx (1)
37-41: LGTM! Test updates reflect component changes.The language display logic is now more explicit and the test coverage remains comprehensive.
src/shared/ui/date-lang/date-lang.tsx (1)
14-19: LGTM! Props refactoring improves component flexibility.The removal of mode prop and addition of children prop enhances reusability.
src/entities/course/ui/course-card/course-card.module.scss (1)
95-126: LGTM! Size variants improve flexibility.The size-sm variant provides a good solution for compact layouts.
src/shared/ui/link-custom/link-custom.module.scss (2)
19-20: LGTM! Button refinements improve consistency.Reduced padding and consistent border styling enhance visual harmony.
72-79: LGTM! Icon wrapper improves alignment.Consistent icon sizing and alignment through wrapper classes.
src/core/styles/_mixins.scss (1)
65-67:❓ Verification inconclusive
Review hover animation performance.
The new hover animation using translate and scale could impact performance on lower-end devices when many cards are present.
Consider testing the hover animation performance with:
🏁 Script executed:
#!/bin/bash # Check for will-change or transform properties that might help with performance rg -g '*.scss' 'will-change|transform:'Length of output: 1359
Investigate Hover Animation Performance
The hover animation in src/core/styles/_mixins.scss (lines 65–67) currently uses
translateandscalewithout awill-changehint. This could lead to suboptimal performance on lower-end devices, especially with many cards on screen. Please verify animation smoothness across devices and consider addingwill-change: transform(or a similar property) if needed to leverage hardware acceleration.src/entities/course/ui/course-card/course-card.tsx (2)
24-26: LGTM! Good addition of size control.The new size prop with 'sm' | 'md' options provides better control over card dimensions, addressing the size-related feedback.
55-55:❓ Verification inconclusive
Verify accessibility with size changes.
The font size changes and conditional text might affect accessibility. Ensure proper contrast ratios and readability are maintained across both size variants.
Also applies to: 58-58, 66-66
🏁 Script executed:
#!/bin/bash # Check for any existing accessibility-related styles rg -g '*.scss' '(font-size|line-height|color).*:.*' src/entities/course/ui/course-card/Length of output: 893
Accessibility Check: Verify Dynamic Styles and Contrast
Please ensure that the dynamic font size changes and conditional text at lines 55, 58, and 66 of
src/entities/course/ui/course-card/course-card.tsxcontinue to meet accessibility standards. The SCSS file (course-card.module.scss) explicitly sets a 14px font size and other styles, so double-check that both size variants maintain proper contrast ratios and readability.
| @@ -57,7 +57,7 @@ export const LinkCustom = ({ | |||
| case variant === 'secondary': | |||
| return <ArrowIcon />; | |||
| case variant === 'rounded': | |||
| return <ArrowIcon size={16} />; | |||
| return <ArrowIcon />; | |||
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Consider maintaining consistent icon sizing.
Removing the size prop for the 'rounded' variant while keeping it for other variants may lead to inconsistent icon sizes across different link styles.
🏁 Script executed:
#!/bin/bash
# Search for other ArrowIcon usages to verify size consistency
rg "ArrowIcon" -A 2 -B 2Length of output: 2174
Fix Icon Sizing for Rounded Variant
- The test expects a small-sized ArrowIcon for the 'rounded' variant, but the component currently renders
<ArrowIcon />without the size prop. - Please ensure that the size prop is explicitly set (e.g.,
<ArrowIcon size="small" />) for the 'rounded' variant or update the test to reflect the new design if intentional.
| align-items: center; | ||
|
|
||
| height: 100%; | ||
| min-height: 112px; | ||
| padding: 0 24px; | ||
| padding: 36px 32px; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Review padding values.
Large padding values (32px, 36px) contribute to increased card size. Consider reducing to maintain compact design:
- padding: 36px 32px;
+ padding: 24px 24px;
- padding: 32px;
+ padding: 24px;Also applies to: 64-64
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