563-refactor: Widget school menu #659
Conversation
|
Lighthouse Report:
|
|
Lighthouse Report:
|
|
Lighthouse Report:
|
Quiddlee
requested review from
andron13,
dzmitry-varabei,
ansivgit and
natanchik
as code owners
There was a problem hiding this comment.
Copilot reviewed 5 out of 20 changed files in this pull request and generated 2 comments.
Files not reviewed (15)
- src/core/base-layout/components/header/header.module.scss: Language not supported
- src/widgets/school-menu/ui/school-item/school-item.module.scss: Language not supported
- src/widgets/school-menu/ui/school-menu.scss: Language not supported
- src/widgets/school-menu/ui/school-menu/school-menu.module.scss: Language not supported
- src/widgets/school-menu/school-menu.test.tsx: Evaluated as low risk
- src/widgets/school-menu/ui/school-menu.tsx: Evaluated as low risk
- src/widgets/school-menu/ui/school-list/school-list.tsx: Evaluated as low risk
- src/core/base-layout/components/header/nav-item/nav-item.tsx: Evaluated as low risk
- dev-data/index.ts: Evaluated as low risk
- src/core/const/index.ts: Evaluated as low risk
- src/shared/tests/visual/main.spec.ts: Evaluated as low risk
- src/widgets/mobile-view/ui/mobile-view.tsx: Evaluated as low risk
- src/widgets/school-menu/index.ts: Evaluated as low risk
- src/core/base-layout/components/header/header.tsx: Evaluated as low risk
- src/widgets/school-menu/types.ts: Evaluated as low risk
Comments suppressed due to low confidence (1)
src/widgets/school-menu/ui/school-menu/school-menu.tsx:16
- The 'className' prop is being passed to the 'ul' element without any validation. Ensure that it is a valid class name.
export const SchoolMenu = ({ heading, color = 'light', children, className }: SchoolMenuProps) => {
|
Lighthouse Report:
|
📝 WalkthroughWalkthroughThe pull request introduces enhancements to the Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🔇 Additional comments (2)src/widgets/school-menu/ui/school-item/school-item.tsx (2)
The Line range hint The component follows React best practices with:
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: 4
🧹 Outside diff range and nitpick comments (12)
src/widgets/school-menu/ui/school-item/school-item.tsx (1)
34-34: Provide meaningfulalttext for theImagecomponentCurrently, the
altattribute is empty (alt=""). To enhance accessibility, consider providing a descriptivealttext if the icon conveys important information.src/widgets/school-menu/ui/school-menu/school-menu.tsx (1)
16-21: Pass down additional props to the<ul>elementSince
SchoolMenuPropsextendsHTMLProps<HTMLUListElement>, it would be beneficial to spread...propsonto the<ul>to allow additional HTML attributes to be applied.dev-data/school-menu-links.ts (1)
5-13: Align property names between data and component propsThe static link objects use
detailsUrl, whileSchoolItemPropsexpects aurlproperty. For consistency and to prevent potential errors, consider renamingdetailsUrltourlin the static link data.Also applies to: 18-36
src/shared/__tests__/visual/main.spec.ts (1)
31-41: Consider enhancing test reliability and assertions.Add visibility checks and improve screenshot naming:
test('Main page desktop menu', async ({ page }) => { await page.goto(ROUTES.HOME); const elements = page.getByTestId('menu-item'); const elementsCount = await elements.count(); + expect(elementsCount).toBeGreaterThan(0); for (let i = 0; i < elementsCount; i++) { await elements.nth(i).hover(); + const menuContent = page.getByTestId(`menu-content-${i}`); + await expect(menuContent).toBeVisible(); - await takeScreenshot(page, `Main page desktop - menu open ${i + 1}`); + await takeScreenshot(page, `Main page desktop - ${await elements.nth(i).textContent()} menu`); } });src/core/base-layout/components/footer/desktop-view.tsx (1)
13-23: Consider lifting color prop to reduce duplicationThe color="light" prop is repeated across all SchoolMenu.Item components. Consider lifting it to the SchoolMenu level.
<SchoolMenu heading="rs school" color="light"> {schoolMenuStaticLinks.map((link) => ( <SchoolMenu.Item key={link.detailsUrl} title={link.title} description={link.description} url={link.detailsUrl} - color="light" /> ))} </SchoolMenu>Also applies to: 27-38
src/core/base-layout/components/header/header.module.scss (1)
95-96: Consider adding fallback for dvh unitWhile
dvhprovides better mobile viewport handling, it needs fallback for older browsers.- min-height: 100dvh; - max-height: 100dvh; + min-height: 100vh; + min-height: 100dvh; + max-height: 100vh; + max-height: 100dvh;src/core/base-layout/components/header/nav-item/nav-item.tsx (1)
67-67: Consider simplifying href constructionThe href template literal can be simplified when only prepending a slash.
-href={`/${href}`} +href={'/' + href}src/widgets/mobile-view/ui/mobile-view.tsx (1)
40-49: Consider extracting repeated SchoolMenu.Item mapping logicThere are three nearly identical mapping patterns for SchoolMenu.Item rendering.
Consider creating a reusable helper:
+ const renderMenuItems = (items: any[], color?: string, onClose?: () => void) => ( + items.map((item, i) => ( + <SchoolMenu.Item + key={item.id || i} + title={item.title} + description={item.description} + url={item.detailsUrl} + icon={item.iconSmall} + color={color} + onClick={onClose} + /> + )) + );Also applies to: 79-88, 98-107
src/widgets/school-menu/ui/school-menu/school-menu.test.tsx (4)
9-12: Consider adding type guards instead of type assertions.Replace type assertions with type guards to improve type safety.
- const aws = mockedCourses.find( - (course) => course.title === COURSE_TITLES.AWS_FUNDAMENTALS, - ) as Course; + const aws = mockedCourses.find( + (course): course is Course => course.title === COURSE_TITLES.AWS_FUNDAMENTALS, + ); + if (!aws) throw new Error('AWS course not found in mocked data');
47-49: Consider using more specific assertions.Instead of checking length only, verify the actual content of links and descriptions.
expect(screen.getAllByRole('link')).toHaveLength(2); - expect(container.getElementsByTagName('small')).toHaveLength(2); + const links = screen.getAllByRole('link'); + expect(links[0]).toHaveTextContent(schoolMenuStaticLinks[0].title); + expect(links[1]).toHaveTextContent(schoolMenuStaticLinks[1].title);
71-89: Improve test description clarity.The test name "[mentorshipId]" is unclear. Consider renaming to better describe what's being tested.
- it('renders [mentorshipId] correct when "all courses" heading is passed', () => { + it('renders course icons correctly for all courses', () => {
126-129: Avoid using magic numbers in array indices.Instead of using hardcoded indices, find elements by their content.
- const linkReact = links.at(3); - const linkAWS = links.at(-1); + const linkReact = screen.getByRole('link', { name: new RegExp(COURSE_TITLES.REACT, 'i') }); + const linkAWS = screen.getByRole('link', { name: new RegExp(COURSE_TITLES.AWS_FUNDAMENTALS, 'i') });
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
dev-data/index.ts(1 hunks)dev-data/school-menu-links.ts(1 hunks)src/core/base-layout/components/footer/desktop-view.tsx(2 hunks)src/core/base-layout/components/header/header.module.scss(1 hunks)src/core/base-layout/components/header/header.tsx(3 hunks)src/core/base-layout/components/header/nav-item/nav-item.tsx(4 hunks)src/core/const/index.ts(1 hunks)src/shared/__tests__/visual/main.spec.ts(1 hunks)src/widgets/mobile-view/ui/mobile-view.tsx(3 hunks)src/widgets/school-menu/index.ts(1 hunks)src/widgets/school-menu/school-menu.test.tsx(0 hunks)src/widgets/school-menu/types.ts(1 hunks)src/widgets/school-menu/ui/school-item/school-item.module.scss(1 hunks)src/widgets/school-menu/ui/school-item/school-item.tsx(1 hunks)src/widgets/school-menu/ui/school-list/school-list.tsx(0 hunks)src/widgets/school-menu/ui/school-menu.scss(0 hunks)src/widgets/school-menu/ui/school-menu.tsx(0 hunks)src/widgets/school-menu/ui/school-menu/school-menu.module.scss(1 hunks)src/widgets/school-menu/ui/school-menu/school-menu.test.tsx(1 hunks)src/widgets/school-menu/ui/school-menu/school-menu.tsx(1 hunks)
💤 Files with no reviewable changes (4)
- src/widgets/school-menu/ui/school-list/school-list.tsx
- src/widgets/school-menu/ui/school-menu.scss
- src/widgets/school-menu/school-menu.test.tsx
- src/widgets/school-menu/ui/school-menu.tsx
✅ Files skipped from review due to trivial changes (2)
- src/widgets/school-menu/index.ts
- src/widgets/school-menu/ui/school-menu/school-menu.module.scss
🔇 Additional comments (9)
src/widgets/school-menu/types.ts (1)
1-1: LGTM!
The new Color type improves type safety and clarifies expected values.
src/core/const/index.ts (1)
5-8: LGTM! New anchor constants follow consistent naming pattern.
src/core/base-layout/components/header/nav-item/nav-item.tsx (1)
Line range hint 60-94: LGTM! Well-structured component with good accessibility
The component has proper keyboard navigation, ARIA attributes, and event handling.
dev-data/index.ts (1)
36-36: LGTM: Clean export of menu links
The export follows the established pattern and supports the centralization of menu data.
src/widgets/mobile-view/ui/mobile-view.tsx (1)
35-37: Consistent onClick handler usage
Good implementation of the onClose handler across all navigation links.
Also applies to: 54-56, 74-76, 93-95
src/core/base-layout/components/header/header.tsx (3)
39-41: Clean implementation of menu close handler
Good separation of concerns with a dedicated handler function.
64-65: Simplified color state management
Good cleanup of the color state management, making it more straightforward.
80-87: Consider consistent item structure across menus
The SchoolMenu.Item implementations vary slightly between different menus (some with icons, some without).
Let's check if this is intentional:
Also applies to: 105-112, 117-124
src/widgets/school-menu/ui/school-menu/school-menu.test.tsx (1)
1-7: LGTM! Well-organized imports.
The imports are properly structured with testing utilities, component under test, and mock data.
|
Lighthouse Report:
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
src/widgets/school-menu/ui/school-item/school-item.tsx (1)
30-30: Enhance Link accessibility with aria-label.Add an aria-label to the Link component to improve screen reader experience.
- <Link href={url} className={cx('school-item')}> + <Link href={url} className={cx('school-item')} aria-label={`${title}${description ? `: ${description}` : ''}`}>
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/widgets/school-menu/ui/school-item/school-item.module.scss(1 hunks)src/widgets/school-menu/ui/school-item/school-item.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/widgets/school-menu/ui/school-item/school-item.module.scss
🔇 Additional comments (2)
src/widgets/school-menu/ui/school-item/school-item.tsx (2)
12-18: LGTM! Well-structured type definition.
The type extends HTMLProps correctly and provides a clear interface with required and optional properties.
24-24: 🛠️ Refactor suggestion
Add type validation for the color prop.
Consider using a union type for allowed color values to prevent invalid colors.
- color?: Color;
+ color?: 'dark' | 'light'; // Add specific allowed valuesLikely invalid or redundant comment.
|
Lighthouse Report:
|
|
run visual now |
What type of PR is this? (select all that apply)
Description
Related Tickets & Documents
Screenshots, Recordings
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
Release Notes
New Features
communityMenuStaticLinksandschoolMenuStaticLinksfor enhanced menu navigation.DesktopViewandMobileViewcomponents to utilize new static links for menu items.SchoolMenucomponent to render school items with customizable headings and colors.Colortype for enforcing specific color options.Bug Fixes
Tests
SchoolMenuand desktop menu functionality.Styles
school-itemandschool-menucomponents to enhance layout and responsiveness.