557-refactor: Widget numbers #585
Conversation
github-actions
bot
changed the title
|
Lighthouse Report:
|
|
run visual now |
|
Lighthouse Report:
|
Update
|
|
Lighthouse Report:
|
|
Shouldn't we apply |
I don't quite get that, i have no wrapping even without |
KristiBo
requested review from
andron13,
dzmitry-varabei,
SpaNb4,
ansivgit and
natanchik
as code owners
📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces several changes, including the addition of a new constant 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
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (6)
src/widgets/numbers/ui/info-cell/info-cell.tsx (1)
12-17: Component structure is clean and follows best practices.The InfoCell component is well-structured and uses semantic HTML. Consider adding ARIA labels for improved accessibility.
Here's a suggested improvement:
export const InfoCell = ({ title, description }: InfoCellProps) => ( - <article className={cx('info-cell')}> + <article className={cx('info-cell')} aria-label="Information Cell"> <div className={cx('info-cell-number')}>{title}</div> <div className={cx('info-cell-text')}>{description}</div> </article> );src/widgets/numbers/ui/info-cell/info-cell.test.tsx (1)
9-13: Assertions cover basic functionality.Good checks for title and description. Consider adding tests for edge cases or different prop types.
src/widgets/numbers/ui/numbers/numbers.test.tsx (1)
10-15: Test case structure is solid.Good use of screen queries. Consider adding more test cases for different scenarios.
src/widgets/numbers/ui/info-cell/info-cell.module.scss (1)
43-51: Consider flexible height for text.Fixed height might truncate longer content. Maybe use min-height instead?
src/widgets/numbers/ui/numbers/numbers.module.scss (1)
27-37: Consider fallback for text-wrap: balance.While text-wrap: balance improves readability, it's not universally supported. Consider adding a fallback for browsers that don't support it.
src/widgets/numbers/ui/numbers/numbers.tsx (1)
15-39: Content rendering looks good, with a minor suggestion.The dynamic rendering of InfoCell components and the use of semantic HTML are commendable. The image includes alt text, which is great for accessibility.
Consider adding aria-label to the section element for improved accessibility:
- <section className={cx('container')}> + <section className={cx('container')} aria-label="The Rolling Scopes community statistics">
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (12)
- dev-data/index.ts (1 hunks)
- dev-data/rs-in-numbers.data.ts (1 hunks)
- src/widgets/numbers/index.ts (1 hunks)
- src/widgets/numbers/numbers.test.tsx (0 hunks)
- src/widgets/numbers/ui/info-cell/info-cell.module.scss (1 hunks)
- src/widgets/numbers/ui/info-cell/info-cell.test.tsx (1 hunks)
- src/widgets/numbers/ui/info-cell/info-cell.tsx (1 hunks)
- src/widgets/numbers/ui/numbers.scss (0 hunks)
- src/widgets/numbers/ui/numbers.tsx (0 hunks)
- src/widgets/numbers/ui/numbers/numbers.module.scss (1 hunks)
- src/widgets/numbers/ui/numbers/numbers.test.tsx (1 hunks)
- src/widgets/numbers/ui/numbers/numbers.tsx (1 hunks)
💤 Files with no reviewable changes (3)
- src/widgets/numbers/numbers.test.tsx
- src/widgets/numbers/ui/numbers.scss
- src/widgets/numbers/ui/numbers.tsx
✅ Files skipped from review due to trivial changes (2)
- dev-data/rs-in-numbers.data.ts
- src/widgets/numbers/index.ts
🧰 Additional context used
🔇 Additional comments (13)
src/widgets/numbers/ui/info-cell/info-cell.tsx (2)
1-5: Imports and setup look good.The use of classnames/bind with CSS modules is a solid choice for managing dynamic class names.
7-10: Type definition is well-structured.The InfoCellProps type clearly defines the expected props, adhering to the PR objective of using types instead of interfaces.
src/widgets/numbers/ui/info-cell/info-cell.test.tsx (2)
1-2: Imports look good.Correct imports for testing library and component.
4-8: Well-structured test setup.Good job defining test data outside and using descriptive test names.
src/widgets/numbers/ui/numbers/numbers.test.tsx (1)
1-8: Imports and test setup look good.Proper imports and test structure using describe and beforeEach.
src/widgets/numbers/ui/info-cell/info-cell.module.scss (4)
1-10: Main container styles look good.Flex layout and dimensions are well-defined.
12-28: Consider alternative for vertical line.The pseudo-element works, but a border might be simpler. Why this approach?
30-41: Number styles are well-defined.Font properties and responsive adjustments look good.
53-62: Responsive adjustments look good.Media queries appropriately handle different screen sizes.
src/widgets/numbers/ui/numbers/numbers.module.scss (2)
1-7: Flexbox layout for centering: Approved.Effective use of flexbox for centering content vertically and horizontally.
9-25: Consider z-index implications.The negative z-index might cause issues if there are other elements with a higher stacking context. Ensure this doesn't interfere with other components.
src/widgets/numbers/ui/numbers/numbers.tsx (1)
1-11: Imports and setup look good.The necessary components and utilities are imported, and CSS modules are properly set up.
dev-data/index.ts (1)
36-36: New export added correctly.The
rsInNumbersexport is consistent with the file's structure.
fixed. The wrapping caused because of different system fonts |
|
Lighthouse Report:
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
src/widgets/numbers/ui/numbers/numbers.module.scss (2)
1-11: Consider responsive text wrapping for .numbers-title.The nowrap might cause overflow on smaller screens. Add a media query for mobile views.
31-41: Add fallback for text-wrap: balance.This property has limited browser support. Consider a fallback solution.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
- src/widgets/numbers/ui/numbers/numbers.module.scss (1 hunks)
- src/widgets/numbers/ui/numbers/numbers.tsx (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/widgets/numbers/ui/numbers/numbers.tsx
🧰 Additional context used
🔇 Additional comments (2)
src/widgets/numbers/ui/numbers/numbers.module.scss (2)
13-29: Responsive map positioning looks good.The absolute positioning and media query adjustments are well-implemented.
43-76: Simplify complex selectors as previously suggested.Use semantic class names instead of nth-child selectors for better maintainability.
What type of PR is this? (select all that apply)
Description
Refactor Numbers widget
Related Tickets & Documents
Screenshots, Recordings
Before
After
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
rsInNumbersconstant for displaying RS School statistics.InfoCellcomponent for structured title and description display.Numberscomponent to present information about "The Rolling Scopes" community.InfoCellandNumberscomponents to enhance layout and responsiveness.InfoCellandNumberscomponents to ensure proper functionality.Numbersentity.