Change pagination styling according to new requirements from designer#153
Change pagination styling according to new requirements from designer#153artem-blazhko wants to merge 1 commit intodevelopfrom
Conversation
WalkthroughAdds a new SCSS class to center pagination controls with spacing and updates the pagination component to use a boolean (isMoreThanOnePages) to conditionally apply that class and render PageControls when totalPages > 1. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as Pagination Component
participant CSS as Stylesheet
UI->>UI: compute isMoreThanOnePages = totalPages > 1
alt isMoreThanOnePages == true
UI->>CSS: apply .pagination_with_controls
UI->>UI: render PageControls
else isMoreThanOnePages == false
UI->>CSS: apply .pagination (base)
UI->>UI: omit PageControls
end
note right of UI #E8F6EF: Primary change is conditional class + render
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/components/pagination/pagination.module.scss (1)
17-20: Consider a more explicit CSS pattern for compound classes.The current approach relies on source order for CSS specificity. While functional, a more robust pattern would explicitly indicate that this class is meant to be used alongside
.pagination.Consider one of these approaches:
Option 1: BEM modifier pattern
-.pagination_with_controls { +.pagination--with-controls { justify-content: center; gap: 24px; }Option 2: Compound selector (higher specificity)
-.pagination_with_controls { +.pagination.pagination_with_controls { justify-content: center; gap: 24px; }Either approach makes the intended usage clearer and less dependent on source order.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/components/pagination/pagination.module.scss(1 hunks)src/components/pagination/pagination.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/components/pagination/pagination.tsx (1)
src/components/pagination/itemsCounter/itemCounter.tsx (1)
ItemCounter(15-30)
🔇 Additional comments (1)
src/components/pagination/pagination.tsx (1)
46-46: Good refactor: Extracting the multi-page condition.Extracting
totalPages > 1into a named boolean variable improves readability and maintainability. The conditional class application and rendering logic are both correct (assuming the typo in the variable name is fixed).Also applies to: 54-54
593c20f to
1a2ceed
Compare
| } | ||
|
|
||
| .pagination_with_controls { | ||
| justify-content: center; | ||
| gap: 24px; | ||
| } |
There was a problem hiding this comment.
| } | |
| .pagination_with_controls { | |
| justify-content: center; | |
| gap: 24px; | |
| } | |
| &.has-controls { | |
| justify-content: center; | |
| gap: 24px; | |
| } | |
| } |
|
|
||
| return ( | ||
| <div className={cx('pagination')}> | ||
| <div className={cx('pagination', isMoreThanOnePages ? 'pagination_with_controls' : '')}> |
There was a problem hiding this comment.
| <div className={cx('pagination', isMoreThanOnePages ? 'pagination_with_controls' : '')}> | |
| <div className={cx('pagination', { | |
| 'pagination_with_controls': isMoreThanOnePages, | |
| )}> |
Summary by CodeRabbit
Style
Refactor