742-refactor: Update subtitle to support heading tags #844
Conversation
oibioib
requested review from
andron13,
dzmitry-varabei,
SpaNb4,
Quiddlee,
ansivgit and
natanchik
as code owners
|
run visual now |
📝 WalkthroughWalkthroughThis change refactors the Changes
Sequence Diagram(s)sequenceDiagram
participant ParentComponent
participant Subtitle
ParentComponent->>Subtitle: Render with as="h2" | "h3" | ... | fontWeight="bold"
Subtitle->>Subtitle: Determine HeadingTag from as prop (default h3)
Subtitle->>Subtitle: Apply font size/weight classes
Subtitle-->>ParentComponent: Rendered heading element with correct tag and styles
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Tip ⚡💬 Agentic Chat (Pro Plan, General Availability)
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (9)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (7)
🪧 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: 0
🧹 Nitpick comments (1)
src/shared/ui/subtitle/subtitle.tsx (1)
9-9: Consider including h1 for completenessThe component supports heading levels h2-h6, but h1 is excluded. Consider whether h1 should be included for completeness, although it's typically reserved for page titles.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
src/app/docs/components/search/search.module.scss(1 hunks)src/app/docs/components/search/search.tsx(2 hunks)src/entities/event/ui/event-card/event-card.module.scss(0 hunks)src/entities/event/ui/event-card/event-card.tsx(2 hunks)src/entities/mentor/ui/mentor-feedback-card/mentor-feedback-card.module.scss(1 hunks)src/entities/mentor/ui/mentor-feedback-card/mentor-feedback-card.tsx(1 hunks)src/entities/trainer/ui/trainers-card/trainer-card.module.scss(0 hunks)src/entities/trainer/ui/trainers-card/trainer-card.tsx(2 hunks)src/shared/ui/modal/modal.module.scss(0 hunks)src/shared/ui/modal/modal.tsx(2 hunks)src/shared/ui/subtitle/subtitle.module.scss(1 hunks)src/shared/ui/subtitle/subtitle.test.tsx(1 hunks)src/shared/ui/subtitle/subtitle.tsx(2 hunks)src/shared/ui/video-playlist-with-player/playlist/playlist.module.scss(0 hunks)src/shared/ui/video-playlist-with-player/playlist/playlist.tsx(2 hunks)src/views/mentorship/mentorship-hero/ui/mentorship-hero.tsx(1 hunks)src/views/mentorship/ui/mentor-activities/ui/activity-card/activity-card.tsx(1 hunks)src/views/mentorship/ui/mentors-after-register/ui/mentors-after-register.module.scss(0 hunks)src/views/mentorship/ui/mentors-after-register/ui/mentors-after-register.tsx(1 hunks)src/widgets/school-menu/ui/school-menu/school-menu.module.scss(0 hunks)src/widgets/school-menu/ui/school-menu/school-menu.tsx(2 hunks)
💤 Files with no reviewable changes (6)
- src/shared/ui/modal/modal.module.scss
- src/shared/ui/video-playlist-with-player/playlist/playlist.module.scss
- src/entities/event/ui/event-card/event-card.module.scss
- src/views/mentorship/ui/mentors-after-register/ui/mentors-after-register.module.scss
- src/entities/trainer/ui/trainers-card/trainer-card.module.scss
- src/widgets/school-menu/ui/school-menu/school-menu.module.scss
🔇 Additional comments (31)
src/app/docs/components/search/search.module.scss (1)
22-23: Added appropriate spacing to search resultsAdding margin-top improves the visual separation between the search input and results.
src/views/mentorship/mentorship-hero/ui/mentorship-hero.tsx (1)
30-30: Updated property name fromweighttofontWeightProperty name change aligns with the subtitle component refactoring.
src/views/mentorship/ui/mentor-activities/ui/activity-card/activity-card.tsx (1)
21-21: Updated property name fromweighttofontWeightConsistent prop naming change for the Subtitle component.
src/entities/trainer/ui/trainers-card/trainer-card.tsx (2)
5-5: Added Subtitle component importAppropriate import added for the new component usage.
19-24: Replaced HTML headings with Subtitle componentEffectively replaced h3/h4 elements with the Subtitle component, properly using the
asprop to maintain semantic heading levels while achieving consistent styling.src/app/docs/components/search/search.tsx (2)
10-10: Import of Subtitle component added correctly.The Subtitle component is properly imported from the shared UI directory.
131-131: Appropriate usage of Subtitle component.Replaced h3 element with Subtitle component using proper fontSize and fontWeight props. This aligns with the standardization goal of the PR.
src/widgets/school-menu/ui/school-menu/school-menu.tsx (2)
4-4: Import of Subtitle component added correctly.The Subtitle component is properly imported from the shared UI directory.
21-21: Appropriate usage of Subtitle component.Correctly replaced the heading element with Subtitle component while preserving the existing className handling.
src/shared/ui/subtitle/subtitle.test.tsx (2)
66-71: Test description clarified for default behavior.Modified test description accurately indicates that h3 is the default rendering tag.
73-99: Comprehensive test coverage for new heading level functionality.Added tests for all heading levels (h2, h4, h5, h6) with proper assertions. This ensures the new
asprop is working correctly for all supported heading tags.src/entities/event/ui/event-card/event-card.tsx (3)
5-5: Import of Subtitle component added correctly.The Subtitle component is properly imported from the shared UI directory.
34-40: Appropriate usage of Subtitle component for organization section.Correctly replaced heading elements with Subtitle components while setting appropriate props:
- Used fontWeight="regular" for organizedBy text
- Used as="h4" with fontSize="extra-small" for organization text
This maintains proper semantic structure while leveraging the new component.
42-46: Appropriate usage of Subtitle component for event title.Correctly replaced the heading element with Subtitle component for the event title while preserving the className.
src/shared/ui/modal/modal.tsx (2)
6-6: Import of Subtitle component added appropriatelyThe Subtitle component is now correctly imported from the relative path.
105-107: Good refactoring to use the Subtitle componentThe modal title now uses the standardized Subtitle component instead of direct HTML h2 element, maintaining the data-testid attribute for testing purposes.
src/views/mentorship/ui/mentors-after-register/ui/mentors-after-register.tsx (2)
36-38: Appropriate Subtitle configurationThe Subtitle component is properly configured with fontSize and fontWeight props, ensuring consistent heading styling.
39-41: Content formatting improvedThe paragraph content is now formatted across multiple lines for better code readability without affecting the rendered output.
src/entities/mentor/ui/mentor-feedback-card/mentor-feedback-card.tsx (2)
42-50: Heading level explicitly defined with as="h4"The Subtitle component now properly specifies the semantic heading level as h4 with appropriate fontWeight and fontSize props, maintaining the className and data-testid attribute.
51-59: Course info converted to Subtitle componentThe course information is now consistently rendered using the Subtitle component with appropriate semantic level (h4) and styling (fontWeight="medium").
src/entities/mentor/ui/mentor-feedback-card/mentor-feedback-card.module.scss (1)
52-52: Line height added for better text readabilityAdding line-height to .card-subtitle improves text readability and vertical spacing.
src/shared/ui/video-playlist-with-player/playlist/playlist.tsx (2)
4-4: Import statement added appropriatelyThe Subtitle component is imported correctly from the relative path.
24-26: Heading element replaced with Subtitle componentThe h3 element has been successfully replaced with the Subtitle component, maintaining the same functionality while aligning with the new standardized approach. The fontSize prop is appropriately set to "small" and all original attributes are preserved.
src/shared/ui/subtitle/subtitle.module.scss (3)
55-57: Class renamed for clarityThe font weight class has been renamed from "normal" to "light-font-weight" to better reflect its purpose and improve naming consistency.
59-61: New font weight class addedA new "regular-font-weight" class has been added, expanding the available font weight options.
67-67: Class renamed for consistencyThe "bold" class has been renamed to "bold-font-weight" to maintain naming consistency with other font weight classes.
src/shared/ui/subtitle/subtitle.tsx (5)
8-10: Added dynamic heading level supportThe SubtitleProps now includes an optional "as" prop, allowing the component to render different heading levels (h2-h6).
23-28: Renamed weight variant for clarityThe "weight" variant has been renamed to "fontWeight" and values have been expanded to include "light", "regular", "medium", and "bold", improving naming clarity.
32-32: Updated default font weightThe default font weight is now explicitly set to "medium".
36-45: Component implementation improvedThe Subtitle component now uses the "as" prop to dynamically determine the heading level, with h3 as the default. The HeadingTag variable is used appropriately to render the correct element.
47-48: Updated prop usage in componentThe component now correctly uses "fontWeight" instead of "weight" and renders the appropriate heading element based on the "as" prop.
Also applies to: 50-50
|
run visual now |
|
Lighthouse Report:
|
|
Lighthouse Report:
|
| <Subtitle | ||
| as="h4" | ||
| fontWeight="bold" | ||
| fontSize="small" | ||
| className={cx('card-title')} | ||
| data-testid="card-title" | ||
| > | ||
| {name} | ||
| </Subtitle> | ||
| <h4 className={cx('card-subtitle')} data-testid="card-subtitle"> | ||
| <Subtitle | ||
| as="h4" | ||
| fontWeight="medium" | ||
| className={cx('card-subtitle')} | ||
| data-testid="card-subtitle" | ||
| > | ||
| <b>Course: </b> | ||
| {course} | ||
| </h4> | ||
| </Subtitle> |
There was a problem hiding this comment.
We could use this:
<Subtitle
as="h3"
fontWeight="bold"
fontSize="small"
className={cx('card-title')}
data-testid="card-title"
>
{name}
</Subtitle>
<Subtitle
as="h4"
fontWeight="medium"
fontSize="extra-small"
className={cx('card-subtitle')}
data-testid="card-subtitle"
>
<b>Course: </b>
{course}
</Subtitle>
And remove some properties from the .card-subtitle class
There was a problem hiding this comment.
for fontSize="extra-small" we have 20px font-size
but here we need font-size: 18px
We don't have such susbtitle font-size.
There was a problem hiding this comment.
I am of the same opinion.
|
Lighthouse Report:
|
|
Lighthouse Report:
|
|
Please, add Playwright screenshots with differences |
| @@ -19,6 +19,8 @@ | |||
| } | |||
|
|
|||
| .results { | |||
| margin-top: 10px; | |||
There was a problem hiding this comment.
Please, remove margin here and increase gap for .docs-content in docs/components/docs-layout/docs-layout.module.scss to 50px
| @@ -127,7 +128,7 @@ function Result({ result }: { result: PagefindSearchResult }) { | |||
| return ( | |||
| <div> | |||
| <Link href={removeHtmlExtension(data.url)}> | |||
| <h3>{data.meta.title}</h3> | |||
| <Subtitle fontSize="extra-small" fontWeight="bold">{data.meta.title}</Subtitle> | |||
There was a problem hiding this comment.
Let's change params to size & weight
There was a problem hiding this comment.
Change Subtitle props
fontSize -> size
fontWeight -> weight
?
| @@ -30,12 +31,18 @@ export const EventCard = ({ | |||
| <div className={cx('card-header')} data-testid="card-header"> | |||
| <p className={cx('event-tag')}>{eventType}</p> | |||
| <section className={cx('about-organization')} data-testid="organization-section"> | |||
| <h4 className={cx('organized-by')}>{organizedBy}</h4> | |||
| <h3 className={cx('event-organization')}>{organization}</h3> | |||
| <Subtitle fontWeight="regular" className={cx('organized-by')}> | |||
There was a problem hiding this comment.
Before changes
After changes
Diffs
| &.light-font-weight { | ||
| font-weight: $font-weight-light; | ||
| } |
There was a problem hiding this comment.
Let's change all class-names to match variable names, e.g.
| &.light-font-weight { | |
| font-weight: $font-weight-light; | |
| } | |
| &.font-weight { | |
| &-light { | |
| font-weight: $font-weight-light; | |
| } | |
| ..... | |
| } |
There was a problem hiding this comment.
Done. Also, I refactored font-size in the same way.
| it('renders as h2 element', () => { | ||
| render(<Subtitle as="h2">H2 Element</Subtitle>); | ||
| const subtitle = screen.getByTestId('subtitle'); | ||
|
|
||
| expect(subtitle.tagName).toBe('H2'); | ||
| }); | ||
|
|
||
| it('renders as h4 element', () => { | ||
| render(<Subtitle as="h4">H4 Element</Subtitle>); | ||
| const subtitle = screen.getByTestId('subtitle'); | ||
|
|
||
| expect(subtitle.tagName).toBe('H4'); | ||
| }); | ||
|
|
||
| it('renders as h5 element', () => { | ||
| render(<Subtitle as="h5">H5 Element</Subtitle>); | ||
| const subtitle = screen.getByTestId('subtitle'); | ||
|
|
||
| expect(subtitle.tagName).toBe('H5'); | ||
| }); | ||
|
|
||
| it('renders as h6 element', () => { | ||
| render(<Subtitle as="h6">H6 Element</Subtitle>); | ||
| const subtitle = screen.getByTestId('subtitle'); | ||
|
|
||
| expect(subtitle.tagName).toBe('H6'); | ||
| }); |
There was a problem hiding this comment.
Please, use it.each instead
| it('renders as h2 element', () => { | |
| render(<Subtitle as="h2">H2 Element</Subtitle>); | |
| const subtitle = screen.getByTestId('subtitle'); | |
| expect(subtitle.tagName).toBe('H2'); | |
| }); | |
| it('renders as h4 element', () => { | |
| render(<Subtitle as="h4">H4 Element</Subtitle>); | |
| const subtitle = screen.getByTestId('subtitle'); | |
| expect(subtitle.tagName).toBe('H4'); | |
| }); | |
| it('renders as h5 element', () => { | |
| render(<Subtitle as="h5">H5 Element</Subtitle>); | |
| const subtitle = screen.getByTestId('subtitle'); | |
| expect(subtitle.tagName).toBe('H5'); | |
| }); | |
| it('renders as h6 element', () => { | |
| render(<Subtitle as="h6">H6 Element</Subtitle>); | |
| const subtitle = screen.getByTestId('subtitle'); | |
| expect(subtitle.tagName).toBe('H6'); | |
| }); | |
| it.each(['h2', 'h3', 'h4'])('renders as %s element', (tag) => { | |
| ..... | |
| }); |
| } | ||
|
|
||
| .card-subtitle { | ||
| margin: 0; | ||
| font-size: 18px; | ||
| font-weight: $font-weight-medium; | ||
| line-height: 1; |
There was a problem hiding this comment.
Let's use line-height as in mentor-feedback-card component
| line-height: 1; | |
| line-height: 1.15; |
| <Subtitle fontWeight="regular" className={cx('organized-by')}> | ||
| {organizedBy} | ||
| </Subtitle> | ||
| <Subtitle as="h4" fontSize="extra-small" className={cx('event-organization')}> |
There was a problem hiding this comment.
Let's use the same order of parameters: as - className- size - weight - data-testid. Please, fix this in all subtitles and other components you add.
| {`${id}. ${subtitle}`} | ||
| </Subtitle> | ||
|
|
||
| <p className={cx('step-content')} data-testid="step-content">{content}</p> | ||
| <p className={cx('step-content')} data-testid="step-content"> |
There was a problem hiding this comment.
As I can see we don't use class step-content and we could remove it
There was a problem hiding this comment.
But this does not apply to the issue of subtitles.
| @@ -7,7 +7,7 @@ | |||
| .heading { | |||
| margin: 0; | |||
| font-size: 12px; | |||
| font-weight: $font-weight-medium; | |||
| line-height: 1.15; | |||
There was a problem hiding this comment.
Why do we need line-height here?
There was a problem hiding this comment.
Before changes here was default h3, with default browser line-height.
<h3 className={cx('heading', color)}>{heading}</h3>}To keep the style of the element the same as before, I added line-height.
src/shared/ui/subtitle/subtitle.tsx
Outdated
| as = 'h3', | ||
| children, | ||
| fontSize, | ||
| fontWeight, | ||
| className, | ||
| ...props |
There was a problem hiding this comment.
| as = 'h3', | |
| children, | |
| fontSize, | |
| fontWeight, | |
| className, | |
| ...props | |
| as = 'h3', | |
| className, | |
| fontSize, | |
| fontWeight, | |
| children, | |
| ...props |
|
On |
|
run visual now |
|
Lighthouse Report:
|
What type of PR is this? (select all that apply)
Description
The component accept props:
Default heading level - h3.
Refactor code and change h3-h4 tags to Subtitle.
Related Tickets & Documents
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
Refactor
Tests