613-feat: Update communication section #616
Conversation
|
Lighthouse Report:
|
Quiddlee
requested review from
andron13,
dzmitry-varabei,
SpaNb4 and
natanchik
as code owners
📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces several updates to the communication-related components within the application. A new constant for a Telegram chat link is added, and corresponding properties for English and Russian text are included in the Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
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 and nitpick comments (3)
dev-data/communication.data.ts (1)
Line range hint
3-19: Consider restructuring communication links for consistency.Currently, Discord links are grouped in an object while Telegram links are separate constants. Consider creating a similar structure for Telegram links to maintain consistency and make future additions easier.
export const RS_DOCS_COMMUNICATION_LINK = 'https://docs.rs.school/#/rs-school-chats'; export const RS_DOCS_TELEGRAM_CHATS_LINK = 'https://docs.rs.school/#/rs-school-chats?id=telegram'; -export const JS_EN_TELEGRAM_CHAT_LINK = 'https://t.me/RSS_EN'; + +export const TELEGRAM_LINKS = { + 'js / front-end en': 'https://t.me/RSS_EN' +}; export const DISCORD_LINKS = { 'js / front-end ru': 'https://discord.com/invite/QvEYg7EaQ4', 'js / front-end en': 'https://discord.com/invite/uW5cCHR',src/widgets/communication/ui/communication.test.tsx (1)
Line range hint
1-52: Test coverage needs expansion for Telegram functionality.The test suite currently only validates Discord-related functionality. Based on the PR objectives of adding Telegram as a communication channel, we need additional test cases to verify:
- Telegram link visibility
- Correct Telegram URL
- Proper rendering of updated text mentioning both platforms
Here's a suggested test structure:
it('should render both Discord and Telegram links correctly', () => { const firstCourse = Object.keys(DISCORD_LINKS)[0] as keyof typeof DISCORD_LINKS; renderWithRouter(<Communication courseName={firstCourse} />); const discordLink = screen.getByTestId('discord-link'); const telegramLink = screen.getByTestId('telegram-link'); expect(discordLink).toBeVisible(); expect(telegramLink).toBeVisible(); expect(telegramLink.getAttribute('href')).toMatch(TELEGRAM_LINK); });src/widgets/communication/ui/communication.tsx (1)
62-64: Consider adding aria-label for external linksWhile the external links are properly marked, consider adding aria-labels to improve accessibility.
- <LinkCustom href={DISCORD_LINKS[courseName]} external data-testid="discord-link"> + <LinkCustom + href={DISCORD_LINKS[courseName]} + external + data-testid="discord-link" + aria-label="Join our Discord channel">Also applies to: 80-82
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (6)
- dev-data/communication.data.ts (1 hunks)
- dev-data/index.ts (1 hunks)
- dev-data/widget-communication.data.ts (2 hunks)
- src/widgets/communication/ui/communication.module.scss (1 hunks)
- src/widgets/communication/ui/communication.test.tsx (1 hunks)
- src/widgets/communication/ui/communication.tsx (4 hunks)
🔇 Additional comments (9)
src/widgets/communication/ui/communication.module.scss (3)
5-15: LGTM: Proper SCSS nesting implementedThe
.communication-wrapperis now correctly nested within.communication-content, improving code organization while maintaining the same visual output.
18-21: Skip comment about padding changes
23-36: 🛠️ Refactor suggestionConsider adding Telegram logo styles
Since the PR objectives mention adding Telegram as a communication channel, consider adding similar styling for the Telegram logo.
Add a new class for Telegram logo with appropriate branding colors:
.discord-logo-wrapper { // ... existing styles ... } + .telegram-logo-wrapper { + flex-shrink: 0; + align-self: center; + width: 250px; + padding: 30px; + background-color: #0088cc; + border-radius: 30px; + + @include media-tablet { + width: 150px; + padding: 15px; + } + }dev-data/communication.data.ts (1)
3-3: LGTM! New Telegram link constant added successfully.The constant follows the established naming convention and contains a valid Telegram chat link format.
src/widgets/communication/ui/communication.test.tsx (1)
46-46: Good improvement using test ID!Using
data-testidis more reliable than text-based queries as it's less prone to breaking when text content changes.dev-data/index.ts (1)
13-13: LGTM! Export aligns with PR objectives.The addition of
JS_EN_TELEGRAM_CHAT_LINKis properly grouped with other communication-related exports.Let's verify the constant's definition:
✅ Verification successful
Constant is properly defined and used in the codebase
The verification shows that:
JS_EN_TELEGRAM_CHAT_LINKis correctly defined incommunication.data.ts- It's properly exported in
index.ts- It's being used in the communication widget component as an external link
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify the constant definition in communication.data.ts rg -A 1 "JS_EN_TELEGRAM_CHAT_LINK" --type tsLength of output: 634
dev-data/widget-communication.data.ts (1)
5-11: Verify content matches requirements.The new English content properly reflects both Discord and Telegram as main communication channels, aligning with PR objectives. However, let's verify the exact text matches across components.
✅ Verification successful
Let me search for the content and its usage to verify the text consistency across components. The previous command failed because the
tsxfile type wasn't recognized, so I'm adjusting the search to focus on TypeScript files and also looking at the actual content and usage of these strings.
Content matches requirements and is properly integrated
The text content is correctly implemented and consistent across components:
- The strings are properly defined in
widget-communication.data.ts- They are correctly imported and used in
communication.tsx- Both Discord and Telegram are mentioned as communication channels
- The content flow is logical: Discord introduction followed by Telegram mention
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify text consistency across components rg -l "Discord.*Telegram|Telegram.*Discord" --type ts --type tsxLength of output: 94
Script:
#!/bin/bash # Search for Discord and Telegram mentions in TypeScript files rg -l "Discord.*Telegram|Telegram.*Discord" --type ts # Show content of the widget communication data file cat dev-data/widget-communication.data.ts # Search for any usage of these strings in the codebase rg "subTitleJs|firstParagraphFirstHalf|firstParagraphFirstHalfJs|discordLink|discordLinkJs|firstParagraphSecondHalf|firstParagraphSecondHalfJs" --type tsLength of output: 5548
src/widgets/communication/ui/communication.tsx (2)
11-11: LGTM: New imports and constants support Telegram integrationAlso applies to: 21-21
68-72: LGTM: Telegram integration matches requirementsThe implementation correctly adds Telegram as an additional communication channel for the JS/FE EN course.
|
Lighthouse Report:
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
src/widgets/communication/ui/communication.tsx (1)
46-47: Consider using the imported course constantInstead of using a string literal, consider using the imported course constant for type safety.
-const isJsEnCourse = courseName === 'js / front-end en'; +const isJsEnCourse = courseName === JS_EN_COURSE;
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- src/widgets/communication/ui/communication.tsx (4 hunks)
🔇 Additional comments (3)
src/widgets/communication/ui/communication.tsx (3)
11-11: LGTM: Telegram link import addedThe import of
JS_EN_TELEGRAM_CHAT_LINKaligns with the PR objective to add Telegram as a communication channel.
30-36: LGTM: New text properties for JS courseThe addition of JS-specific text properties (
subTitleJs,firstParagraphFirstHalfJs, etc.) supports the multilingual content requirements.
57-74: Reduce code duplication in conditional renderingA previous review already suggested a refactor to reduce code duplication in this section. The suggestion remains valid and would improve maintainability.
| <LinkCustom href={DISCORD_LINKS[courseName]} external> | ||
| {discordLink} | ||
| <Subtitle className={cx('communication-subtitle')}> | ||
| {isJsEnCourse ? subTitleJs : subTitle} |
There was a problem hiding this comment.
please, move all the conditions above the return statement
const courseSubTitle = isJsEnCourse ? subTitleJs : subTitle;| {isJsEnCourse ? subTitleJs : subTitle} | |
| {courseSubTitle} |
|
Lighthouse Report:
|
What type of PR is this? (select all that apply)
Description
Related Tickets & Documents
Screenshots, Recordings
Before:
After:
Added/updated tests?
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Style