feat: RecomendationTraining component#86
Merged
Merged
Conversation
Collaborator
Author
|
@Tamir198 This is only part of LifStyle page... |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (11)
✅ Files skipped from review due to trivial changes (3)
🚧 Files skipped from review as they are similar to previous changes (5)
📝 WalkthroughWalkthroughAdds a responsive RecomendationTraining feature (mobile + desktop), new layout styles, two video-duration constants, localized ChangesLifestyle Recommendations and Responsive Layout
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/pages/lifeStyle/physioAndTrainingTab/recomendationTraining/style.ts (1)
3-27: ⚡ Quick winPrefer type annotation over
as CSSPropertiescast.Casting with
assilences the type checker, so a misspelled property name or invalid value would slip through. Annotating the variable type validates the object literal while still allowing literal narrowing forflexDirection.♻️ Proposed change (apply to all four objects)
-export const rootStyle_mobile = { +export const rootStyle_mobile: CSSProperties = { display: 'flex', flexDirection: 'column', gap: '1rem', padding: '1rem', paddingBottom: '1rem', -} as CSSProperties +}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pages/lifeStyle/physioAndTrainingTab/recomendationTraining/style.ts` around lines 3 - 27, Replace the "as CSSProperties" casts with explicit type annotations on each exported style object so the object literals are type-checked; specifically change the declarations for rootStyle_mobile, rootStyle_desktop, videoConteiner_desktop, and header_desktop to use ": CSSProperties" after the variable name and retain the same object bodies, applying this pattern to all four exports to catch misspelled or invalid CSS properties at compile time.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/pages/lifeStyle/physioAndTrainingTab/recomendationTraining/style.ts`:
- Around line 24-27: The header_desktop style uses mixed units for vertical
padding (paddingTop: '0.5em' vs paddingBottom: '0.5rem'); make them consistent
by updating one to match the other (e.g., change paddingTop to '0.5rem' or
paddingBottom to '0.5em') so vertical spacing is predictable; locate the
header_desktop export in the style.ts and normalize both paddingTop and
paddingBottom to the same unit.
---
Nitpick comments:
In `@src/pages/lifeStyle/physioAndTrainingTab/recomendationTraining/style.ts`:
- Around line 3-27: Replace the "as CSSProperties" casts with explicit type
annotations on each exported style object so the object literals are
type-checked; specifically change the declarations for rootStyle_mobile,
rootStyle_desktop, videoConteiner_desktop, and header_desktop to use ":
CSSProperties" after the variable name and retain the same object bodies,
applying this pattern to all four exports to catch misspelled or invalid CSS
properties at compile time.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7e0e1f4b-3c67-4a2e-aae3-ee69f5690677
📒 Files selected for processing (8)
src/components/UI/Video/SGLVideoCard.tsxsrc/locales/ar/translation.jsonsrc/locales/en/translation.jsonsrc/locales/he/translation.jsonsrc/locales/ru/translation.jsonsrc/pages/lifeStyle/LifeStyle.tsxsrc/pages/lifeStyle/physioAndTrainingTab/recomendationTraining/RecomendationTraining.tsxsrc/pages/lifeStyle/physioAndTrainingTab/recomendationTraining/style.ts
Comment on lines
+24
to
+27
| export const header_desktop = { | ||
| paddingTop: '0.5em', | ||
| paddingBottom: '0.5rem', | ||
| } as CSSProperties |
There was a problem hiding this comment.
Inconsistent unit: 0.5em vs 0.5rem.
paddingTop uses em while paddingBottom uses rem, which yields asymmetric vertical padding whenever the element's font-size differs from the root. Likely unintended.
🔧 Proposed fix
export const header_desktop = {
- paddingTop: '0.5em',
+ paddingTop: '0.5rem',
paddingBottom: '0.5rem',
} as CSSProperties📝 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.
Suggested change
| export const header_desktop = { | |
| paddingTop: '0.5em', | |
| paddingBottom: '0.5rem', | |
| } as CSSProperties | |
| export const header_desktop = { | |
| paddingTop: '0.5rem', | |
| paddingBottom: '0.5rem', | |
| } as CSSProperties |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/pages/lifeStyle/physioAndTrainingTab/recomendationTraining/style.ts`
around lines 24 - 27, The header_desktop style uses mixed units for vertical
padding (paddingTop: '0.5em' vs paddingBottom: '0.5rem'); make them consistent
by updating one to match the other (e.g., change paddingTop to '0.5rem' or
paddingBottom to '0.5em') so vertical spacing is predictable; locate the
header_desktop export in the style.ts and normalize both paddingTop and
paddingBottom to the same unit.
Tamir198
requested changes
May 30, 2026
| return ( | ||
| <> | ||
| {isMobile ? ( | ||
| <div style={rootStyle_mobile}> |
Member
There was a problem hiding this comment.
You should extract the mobile and desktop into a separate component
| <SGLVideoCard | ||
| title={t('lifeStyle.balanceTrainingTitle')} | ||
| description={t('lifeStyle.balanceTrainingDescription')} | ||
| durationInMinutes={15} |
Member
There was a problem hiding this comment.
no magic numbers, save this into constant value
| <SGLVideoCard | ||
| title={t('lifeStyle.morningStretchingTitle')} | ||
| description={t('lifeStyle.morningStretchingDescription')} | ||
| durationInMinutes={10} |
Member
There was a problem hiding this comment.
no magic numbers, save this into constant value
Collaborator
Author
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/pages/lifeStyle/physioAndTrainingTab/recomendationTraining/RecomendationTrainingMobile.tsx (1)
12-32: ⚡ Quick winRemove unnecessary Fragment wrapper.
The Fragment
<></>wraps a singledivelement. Since there's only one root element, the Fragment is unnecessary.♻️ Proposed simplification
export const RecomendationTrainingMobile = () => { const { t } = useTranslation() return ( - <> - <div style={rootStyle_mobile}> + <div style={rootStyle_mobile}> <SGLTypography variant="mediumTitle" color="black"> {t('lifeStyle.todayRecommendations')} </SGLTypography> <SGLVideoCard title={t('lifeStyle.morningStretchingTitle')} description={t('lifeStyle.morningStretchingDescription')} durationInMinutes={STRETCH_VIDEO_TIME} /> <SGLVideoCard title={t('lifeStyle.balanceTrainingTitle')} description={t('lifeStyle.balanceTrainingDescription')} durationInMinutes={BALANCE_VIDEO_TIME} /> <SGLTypography variant="mediumTitle" color="black"> {t('lifeStyle.balanceExercises')} </SGLTypography> <BalanceTraining /> - </div> - </> + </div> ) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pages/lifeStyle/physioAndTrainingTab/recomendationTraining/RecomendationTrainingMobile.tsx` around lines 12 - 32, Remove the unnecessary Fragment wrapper around the single root div in RecomendationTrainingMobile.tsx: delete the opening <> and closing </> that envelop the div styled by rootStyle_mobile so the component returns the div directly; verify components referenced (SGLTypography, SGLVideoCard, BalanceTraining) and props (STRETCH_VIDEO_TIME, BALANCE_VIDEO_TIME) remain unchanged and tests/exports still pass.src/pages/lifeStyle/physioAndTrainingTab/recomendationTraining/RecomendationTraining.tsx (1)
7-7: ⚡ Quick winRemove unnecessary Fragment wrapper.
The Fragment
<></>wraps a single ternary expression. You can return the ternary directly.♻️ Proposed simplification
export const RecomendationTraining = () => { const isMobile = useIsMobile() - return <>{isMobile ? <RecomendationTrainingMobile /> : <RecomendationTrainingDesktop />}</> + return isMobile ? <RecomendationTrainingMobile /> : <RecomendationTrainingDesktop /> }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pages/lifeStyle/physioAndTrainingTab/recomendationTraining/RecomendationTraining.tsx` at line 7, The return value in the RecomendationTraining component currently wraps a single ternary in a Fragment; remove the unnecessary Fragment and return the ternary expression directly (replace "return <>{isMobile ? <RecomendationTrainingMobile /> : <RecomendationTrainingDesktop />}</>" with a direct return of the ternary). Update the RecomendationTraining function so it returns isMobile ? <RecomendationTrainingMobile /> : <RecomendationTrainingDesktop /> and keep the existing isMobile, RecomendationTrainingMobile and RecomendationTrainingDesktop identifiers unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/pages/lifeStyle/physioAndTrainingTab/recomendationTraining/style.ts`:
- Line 19: Rename the exported constant conteiner_desktop to container_desktop
in style.ts and update all references/imports; specifically change the export
name export const conteiner_desktop -> container_desktop, then update the import
in RecomendationTrainingDesktop.tsx to import container_desktop and replace any
usages of conteiner_desktop in that component (e.g., at the previously noted
lines around the root usage and the other occurrence) to the new symbol
container_desktop so names stay consistent.
---
Nitpick comments:
In
`@src/pages/lifeStyle/physioAndTrainingTab/recomendationTraining/RecomendationTraining.tsx`:
- Line 7: The return value in the RecomendationTraining component currently
wraps a single ternary in a Fragment; remove the unnecessary Fragment and return
the ternary expression directly (replace "return <>{isMobile ?
<RecomendationTrainingMobile /> : <RecomendationTrainingDesktop />}</>" with a
direct return of the ternary). Update the RecomendationTraining function so it
returns isMobile ? <RecomendationTrainingMobile /> :
<RecomendationTrainingDesktop /> and keep the existing isMobile,
RecomendationTrainingMobile and RecomendationTrainingDesktop identifiers
unchanged.
In
`@src/pages/lifeStyle/physioAndTrainingTab/recomendationTraining/RecomendationTrainingMobile.tsx`:
- Around line 12-32: Remove the unnecessary Fragment wrapper around the single
root div in RecomendationTrainingMobile.tsx: delete the opening <> and closing
</> that envelop the div styled by rootStyle_mobile so the component returns the
div directly; verify components referenced (SGLTypography, SGLVideoCard,
BalanceTraining) and props (STRETCH_VIDEO_TIME, BALANCE_VIDEO_TIME) remain
unchanged and tests/exports still pass.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4d090d11-28af-4dd1-bc2d-4c30862d752a
📒 Files selected for processing (6)
src/constants/index.tssrc/pages/lifeStyle/LifeStyle.tsxsrc/pages/lifeStyle/physioAndTrainingTab/recomendationTraining/RecomendationTraining.tsxsrc/pages/lifeStyle/physioAndTrainingTab/recomendationTraining/RecomendationTrainingDesktop.tsxsrc/pages/lifeStyle/physioAndTrainingTab/recomendationTraining/RecomendationTrainingMobile.tsxsrc/pages/lifeStyle/physioAndTrainingTab/recomendationTraining/style.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/pages/lifeStyle/LifeStyle.tsx
| paddingBottom: '1rem', | ||
| } as CSSProperties | ||
|
|
||
| export const conteiner_desktop = { |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Fix typo: conteiner_desktop should be container_desktop.
The export name contains a spelling error. Correct it to container_desktop for consistency with standard English spelling and better code maintainability.
📝 Proposed fix
-export const conteiner_desktop = {
+export const container_desktop = {
display: 'flex',
flexDirection: 'column',
flex: 1,
gap: '1rem',
paddingTop: '1rem',
} as CSSPropertiesNote: You'll also need to update the import in RecomendationTrainingDesktop.tsx:
import {
rootStyle_desktop,
container_desktop, // updated
header_desktop,
videoContainer_desktop,
} from './style'And update both usages in the component (lines 18 and 37).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/pages/lifeStyle/physioAndTrainingTab/recomendationTraining/style.ts` at
line 19, Rename the exported constant conteiner_desktop to container_desktop in
style.ts and update all references/imports; specifically change the export name
export const conteiner_desktop -> container_desktop, then update the import in
RecomendationTrainingDesktop.tsx to import container_desktop and replace any
usages of conteiner_desktop in that component (e.g., at the previously noted
lines around the root usage and the other occurrence) to the new symbol
container_desktop so names stay consistent.
Member
There was a problem hiding this comment.
@rachelishteren I agree with this comment also we write our classs names likeThis and not like_this
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
Tamir198
requested changes
Jun 1, 2026
Tamir198
left a comment
Tamir198
left a comment
Member
There was a problem hiding this comment.
Left comments, also you have conflicts
| export const RecomendationTraining = () => { | ||
| const isMobile = useIsMobile() | ||
|
|
||
| return <>{isMobile ? <RecomendationTrainingMobile /> : <RecomendationTrainingDesktop />}</> |
Member
There was a problem hiding this comment.
How about return isMobile ? <RecomendationTrainingMobile /> : <RecomendationTrainingDesktop />
Collaborator
Author
There was a problem hiding this comment.
I dont understand the comment :)
Member
There was a problem hiding this comment.
I suggested alternative instead of wrapping everything in fragment and {}
| paddingBottom: '1rem', | ||
| } as CSSProperties | ||
|
|
||
| export const conteiner_desktop = { |
Member
There was a problem hiding this comment.
@rachelishteren I agree with this comment also we write our classs names likeThis and not like_this
Collaborator
Author
Tamir198
previously approved these changes
Jun 3, 2026
Tamir198
left a comment
Tamir198
left a comment
Member
There was a problem hiding this comment.
Approved, fix your conflicts
rachelishteren
force-pushed
the
LifeStyle/Recomend
branch
from
f37d483 to
a12126b
Compare
Collaborator
Author
|
@Tamir198 Ready to merge. |
Tamir198
approved these changes
Jun 4, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
feat: RecomendationTraining component - pages/lifeStyle/physioAndTrainingTab/recomendationTraining
Mobile and desktop version support - with a design that is compatible with everyone
Summary by CodeRabbit
New Features
Localization
Style