From d5745e210a9e7a57bb51ca229f4eebd00b365501 Mon Sep 17 00:00:00 2001 From: Peter Kulko Date: Wed, 30 Oct 2024 14:10:20 +0200 Subject: [PATCH] refactor: after review --- package-lock.json | 3 +- package.json | 3 +- src/studio-header/BrandNav.jsx | 10 +--- src/studio-header/BrandNav.test.jsx | 70 +++++++--------------- src/studio-header/CourseLockUp.jsx | 11 ++-- src/studio-header/CourseLockUp.test.jsx | 51 +++++----------- src/studio-header/HeaderBody.jsx | 7 +-- src/studio-header/HeaderBody.test.jsx | 51 +++++++--------- src/studio-header/MobileHeader.jsx | 5 +- src/studio-header/MobileMenu.jsx | 18 ++---- src/studio-header/MobileMenu.test.jsx | 37 ++++++------ src/studio-header/NavDropdownMenu.jsx | 9 +-- src/studio-header/NavDropdownMenu.test.jsx | 45 +++++++------- src/studio-header/StudioHeader.jsx | 4 +- src/studio-header/StudioHeader.test.jsx | 22 ++++--- src/studio-header/UserMenu.jsx | 3 - src/studio-header/utils.js | 25 -------- 17 files changed, 132 insertions(+), 242 deletions(-) diff --git a/package-lock.json b/package-lock.json index 80c9c8121..e520ee8c0 100644 --- a/package-lock.json +++ b/package-lock.json @@ -49,7 +49,8 @@ "@openedx/paragon": ">= 21.5.7 < 23.0.0", "prop-types": "^15.5.10", "react": "^16.9.0 || ^17.0.0", - "react-dom": "^16.9.0 || ^17.0.0" + "react-dom": "^16.9.0 || ^17.0.0", + "react-router-dom": "^6.14.2" } }, "node_modules/@adobe/css-tools": { diff --git a/package.json b/package.json index 35eaa9ab2..7d2106d13 100644 --- a/package.json +++ b/package.json @@ -73,6 +73,7 @@ "@openedx/paragon": ">= 21.5.7 < 23.0.0", "prop-types": "^15.5.10", "react": "^16.9.0 || ^17.0.0", - "react-dom": "^16.9.0 || ^17.0.0" + "react-dom": "^16.9.0 || ^17.0.0", + "react-router-dom": "^6.14.2" } } diff --git a/src/studio-header/BrandNav.jsx b/src/studio-header/BrandNav.jsx index 18c960f10..fd9ecd026 100644 --- a/src/studio-header/BrandNav.jsx +++ b/src/studio-header/BrandNav.jsx @@ -1,29 +1,25 @@ import React from 'react'; import PropTypes from 'prop-types'; -import { Hyperlink } from '@openedx/paragon'; - -import { navigateToUrl } from './utils'; +import { Link } from 'react-router-dom'; const BrandNav = ({ studioBaseUrl, logo, logoAltText, - onNavigate, }) => ( - navigateToUrl(e, studioBaseUrl, onNavigate)}> + {logoAltText} - + ); BrandNav.propTypes = { studioBaseUrl: PropTypes.string.isRequired, logo: PropTypes.string.isRequired, logoAltText: PropTypes.string.isRequired, - onNavigate: PropTypes.func.isRequired, }; export default BrandNav; diff --git a/src/studio-header/BrandNav.test.jsx b/src/studio-header/BrandNav.test.jsx index 805019641..7ea2d3e81 100644 --- a/src/studio-header/BrandNav.test.jsx +++ b/src/studio-header/BrandNav.test.jsx @@ -1,68 +1,40 @@ import React from 'react'; -import { render, fireEvent, screen } from '@testing-library/react'; +import { render, screen } from '@testing-library/react'; import '@testing-library/jest-dom/extend-expect'; +import { MemoryRouter } from 'react-router-dom'; + import BrandNav from './BrandNav'; -describe('BrandNav Component', () => { - const mockOnNavigate = jest.fn(); - const studioBaseUrl = 'https://example.com'; - const logo = 'logo.png'; - const logoAltText = 'Example Logo'; +const studioBaseUrl = 'https://example.com/'; +const logo = 'logo.png'; +const logoAltText = 'Example Logo'; + +const RootWrapper = () => ( + + + +); +describe('BrandNav Component', () => { afterEach(() => { jest.clearAllMocks(); }); it('renders the logo with the correct alt text', () => { - render( - , - ); + render(); const img = screen.getByAltText(logoAltText); - expect(img).toBeInTheDocument(); expect(img).toHaveAttribute('src', logo); }); - it('navigates to an absolute URL by changing window location', () => { - // Mock window.location.href - delete window.location; - window.location = { href: '' }; - - render( - , - ); - - const link = screen.getByRole('link'); - fireEvent.click(link); - - expect(window.location.href).toBe(studioBaseUrl); - }); - - it('calls onNavigate for relative URLs', () => { - const relativeUrl = '/studio/dashboard'; - - render( - , - ); + it('displays a link that navigates to studioBaseUrl', () => { + render(); const link = screen.getByRole('link'); - fireEvent.click(link); - - expect(mockOnNavigate).toHaveBeenCalledWith(relativeUrl); + expect(link.href).toBe(studioBaseUrl); }); }); diff --git a/src/studio-header/CourseLockUp.jsx b/src/studio-header/CourseLockUp.jsx index 7de7f2c1c..c6236147e 100644 --- a/src/studio-header/CourseLockUp.jsx +++ b/src/studio-header/CourseLockUp.jsx @@ -5,8 +5,8 @@ import { OverlayTrigger, Tooltip, } from '@openedx/paragon'; +import { Link } from 'react-router-dom'; -import { navigateToUrl } from './utils'; import messages from './messages'; const CourseLockUp = ({ @@ -14,7 +14,6 @@ const CourseLockUp = ({ org, number, title, - onNavigate, // injected intl, }) => ( @@ -26,16 +25,15 @@ const CourseLockUp = ({ )} > - navigateToUrl(e, outlineLink, onNavigate)} + to={outlineLink} aria-label={intl.formatMessage(messages['header.label.courseOutline'])} data-testid="course-lock-up-block" > {org} {number} {title} - + ); @@ -44,7 +42,6 @@ CourseLockUp.propTypes = { org: PropTypes.string, title: PropTypes.string, outlineLink: PropTypes.string, - onNavigate: PropTypes.func.isRequired, // injected intl: intlShape.isRequired, }; diff --git a/src/studio-header/CourseLockUp.test.jsx b/src/studio-header/CourseLockUp.test.jsx index 614015b25..5dfc48f1f 100644 --- a/src/studio-header/CourseLockUp.test.jsx +++ b/src/studio-header/CourseLockUp.test.jsx @@ -1,35 +1,34 @@ import React from 'react'; -import { render, fireEvent, screen } from '@testing-library/react'; +import { render, screen } from '@testing-library/react'; import '@testing-library/jest-dom/extend-expect'; import { IntlProvider } from '@edx/frontend-platform/i18n'; +import { MemoryRouter } from 'react-router-dom'; import CourseLockUp from './CourseLockUp'; import messages from './messages'; -const mockOnNavigate = jest.fn(); const mockProps = { number: '101', org: 'EDX', title: 'Course Title', outlineLink: 'https://example.com/course-outline', - onNavigate: mockOnNavigate, }; +const RootWrapper = (props) => ( + + + + + +); + describe('CourseLockUp Component', () => { afterEach(() => { jest.clearAllMocks(); }); - const renderWithIntl = (component) => ( - render( - - {component} - , - ) - ); - it('renders course org, number, and title', () => { - renderWithIntl(); + render(); const courseOrgNumber = screen.getByTestId('course-org-number'); const courseTitle = screen.getByTestId('course-title'); @@ -41,7 +40,7 @@ describe('CourseLockUp Component', () => { }); it('renders the link with correct aria-label', () => { - renderWithIntl(); + render(); const link = screen.getByTestId('course-lock-up-block'); expect(link).toHaveAttribute( @@ -51,31 +50,9 @@ describe('CourseLockUp Component', () => { }); it('navigates to an absolute URL when clicked', () => { - // Mock window.location.href - delete window.location; - window.location = { href: '' }; - - renderWithIntl(); + render(); const link = screen.getByTestId('course-lock-up-block'); - fireEvent.click(link); - - expect(window.location.href).toBe(mockProps.outlineLink); - }); - - it('calls onNavigate for relative URLs', () => { - const relativeUrl = '/courses/edx/course-v1-101/'; - - renderWithIntl( - , - ); - - const link = screen.getByTestId('course-lock-up-block'); - fireEvent.click(link); - - expect(mockOnNavigate).toHaveBeenCalledWith(relativeUrl); + expect(link.href).toBe(mockProps.outlineLink); }); }); diff --git a/src/studio-header/HeaderBody.jsx b/src/studio-header/HeaderBody.jsx index 9ef89fe78..c030c413c 100644 --- a/src/studio-header/HeaderBody.jsx +++ b/src/studio-header/HeaderBody.jsx @@ -39,7 +39,6 @@ const HeaderBody = ({ outlineLink, searchButtonAction, containerProps, - onNavigate, }) => { const intl = useIntl(); @@ -49,7 +48,6 @@ const HeaderBody = ({ studioBaseUrl, logo, logoAltText, - onNavigate, }} /> ); @@ -90,7 +88,6 @@ const HeaderBody = ({ number, org, title, - onNavigate, }} /> @@ -109,7 +106,7 @@ const HeaderBody = ({ ); @@ -138,7 +135,6 @@ const HeaderBody = ({ logoutUrl, authenticatedUserAvatar, isAdmin, - onNavigate, }} /> @@ -174,7 +170,6 @@ HeaderBody.propTypes = { outlineLink: PropTypes.string, searchButtonAction: PropTypes.func, containerProps: PropTypes.shape(Container.propTypes), - onNavigate: PropTypes.func.isRequired, onLogoNavigate: PropTypes.func.isRequired, }; diff --git a/src/studio-header/HeaderBody.test.jsx b/src/studio-header/HeaderBody.test.jsx index 5033a6164..6e5a5e69f 100644 --- a/src/studio-header/HeaderBody.test.jsx +++ b/src/studio-header/HeaderBody.test.jsx @@ -2,6 +2,7 @@ import React from 'react'; import { render, screen, fireEvent } from '@testing-library/react'; import '@testing-library/jest-dom/extend-expect'; import { IntlProvider } from '@edx/frontend-platform/i18n'; +import { MemoryRouter } from 'react-router-dom'; import HeaderBody from './HeaderBody'; import messages from './messages'; @@ -32,12 +33,12 @@ const defaultProps = { outlineLink: '/courses/edx/course-101', }; -const renderWithIntl = (component) => ( - render( +const RootWrapper = (props) => ( + - {component} - , - ) + + + ); describe('HeaderBody Component', () => { @@ -46,34 +47,32 @@ describe('HeaderBody Component', () => { }); it('renders the logo and brand navigation', () => { - renderWithIntl(); + render(); - const logoImage = screen.getByAltText('Test Logo'); + const logoImage = screen.getByAltText(defaultProps.logoAltText); expect(logoImage).toBeInTheDocument(); - expect(logoImage).toHaveAttribute('src', 'logo.png'); + expect(logoImage).toHaveAttribute('src', defaultProps.logo); }); it('renders course lockup information', () => { - renderWithIntl(); + render(); - const courseTitle = screen.getByText('Test Course'); - const courseOrgNumber = screen.getByText('EDX 101'); + const courseTitle = screen.getByText(defaultProps.title); + const courseOrgNumber = screen.getByText(`${defaultProps.org} ${defaultProps.number}`); expect(courseTitle).toBeInTheDocument(); expect(courseOrgNumber).toBeInTheDocument(); }); - it('calls onNavigate when course lockup is clicked', () => { - renderWithIntl(); + it('renders a course lock-up link with the correct outline URL', () => { + render(); - const courseLockUpLink = screen.getByText('Test Course'); - fireEvent.click(courseLockUpLink); - - expect(mockOnNavigate).toHaveBeenCalledWith('/courses/edx/course-101'); + const courseLockUpLink = screen.getByTestId('course-lock-up-block'); + expect(courseLockUpLink.getAttribute('href')).toBe(defaultProps.outlineLink); }); it('displays search button and triggers searchButtonAction on click', () => { - renderWithIntl(); + render(); const searchButton = screen.getByLabelText(messages['header.label.search.nav'].defaultMessage); expect(searchButton).toBeInTheDocument(); @@ -83,23 +82,17 @@ describe('HeaderBody Component', () => { }); it('displays user menu with username and avatar', () => { - renderWithIntl(); + render(); - const userMenu = screen.getByText('testuser'); - const avatarImage = screen.getByAltText('testuser'); + const userMenu = screen.getByText(defaultProps.username); + const avatarImage = screen.getByAltText(defaultProps.username); expect(userMenu).toBeInTheDocument(); - expect(avatarImage).toHaveAttribute('src', 'avatar.png'); + expect(avatarImage).toHaveAttribute('src', defaultProps.authenticatedUserAvatar); }); it('toggles mobile menu popup when button is clicked in mobile view', () => { - renderWithIntl( - , - ); + render(); const menuButton = screen.getByTestId('mobile-menu-button'); fireEvent.click(menuButton); diff --git a/src/studio-header/MobileHeader.jsx b/src/studio-header/MobileHeader.jsx index fcfaf22f3..fd3cca1bf 100644 --- a/src/studio-header/MobileHeader.jsx +++ b/src/studio-header/MobileHeader.jsx @@ -6,7 +6,6 @@ import MobileMenu from './MobileMenu'; const MobileHeader = ({ mainMenuDropdowns, - onNavigate, ...props }) => { const [isOpen, , close, toggle] = useToggle(false); @@ -20,7 +19,6 @@ const MobileHeader = ({ setModalPopupTarget={setTarget} toggleModalPopup={toggle} isModalPopupOpen={isOpen} - onNavigate={onNavigate} /> - + ); @@ -57,7 +55,6 @@ MobileHeader.propTypes = { })), })), outlineLink: PropTypes.string, - onNavigate: PropTypes.func.isRequired, }; MobileHeader.defaultProps = { diff --git a/src/studio-header/MobileMenu.jsx b/src/studio-header/MobileMenu.jsx index 462430b2e..892f60381 100644 --- a/src/studio-header/MobileMenu.jsx +++ b/src/studio-header/MobileMenu.jsx @@ -1,13 +1,9 @@ import React from 'react'; import PropTypes from 'prop-types'; -import { Collapsible, Hyperlink } from '@openedx/paragon'; +import { Collapsible } from '@openedx/paragon'; +import { Link } from 'react-router-dom'; -import { navigateToUrl } from './utils'; - -const MobileMenu = ({ - mainMenuDropdowns, - onNavigate, -}) => ( +const MobileMenu = ({ mainMenuDropdowns }) => (
{items.map(item => (
  • - navigateToUrl(e, item.href, onNavigate)} - destination={item.href.startsWith('#') ? item.href : null} - > + {item.title} - +
  • ))} @@ -49,7 +42,6 @@ MobileMenu.propTypes = { title: PropTypes.node, })), })), - onNavigate: PropTypes.func.isRequired, }; MobileMenu.defaultProps = { mainMenuDropdowns: [], diff --git a/src/studio-header/MobileMenu.test.jsx b/src/studio-header/MobileMenu.test.jsx index a7fb3cbb4..38041bee6 100644 --- a/src/studio-header/MobileMenu.test.jsx +++ b/src/studio-header/MobileMenu.test.jsx @@ -1,5 +1,7 @@ import React from 'react'; import { render, screen, fireEvent } from '@testing-library/react'; +import { MemoryRouter } from 'react-router-dom'; + import '@testing-library/jest-dom/extend-expect'; import MobileMenu from './MobileMenu'; @@ -26,50 +28,49 @@ const defaultProps = { onNavigate: mockOnNavigate, }; +const RootWrapper = (props) => ( + + + +); + describe('MobileMenu Component', () => { afterEach(() => { jest.clearAllMocks(); }); test('renders the mobile menu with dropdowns and items', () => { - render(); + render(); const menu1Title = screen.getByText('Menu 1'); const menu2Title = screen.getByText('Menu 2'); + expect(menu1Title).toBeInTheDocument(); expect(menu2Title).toBeInTheDocument(); }); test('navigates to internal URL when item is clicked', () => { - render(); + render(); - const menu1Title = screen.getByText('Menu 1'); + const menu1Title = screen.getByText(defaultProps.mainMenuDropdowns[0].buttonTitle); fireEvent.click(menu1Title); - const menuItem = screen.getByText('Item 1'); - fireEvent.click(menuItem); - - expect(mockOnNavigate).toHaveBeenCalledWith('/menu1/item1'); + const menuItem = screen.getByText(defaultProps.mainMenuDropdowns[0].items[0].title); + expect(menuItem.getAttribute('href')).toBe(defaultProps.mainMenuDropdowns[0].items[0].href); }); test('navigates to an external URL when external link is clicked', () => { - // Mock window.location.href - delete window.location; - window.location = { href: '' }; - - render(); + render(); - const menu2Title = screen.getByText('Menu 2'); + const menu2Title = screen.getByText(defaultProps.mainMenuDropdowns[1].buttonTitle); fireEvent.click(menu2Title); - const externalLink = screen.getByText('External Link'); - fireEvent.click(externalLink); - - expect(window.location.href).toBe('https://external-link.com'); + const externalLink = screen.getByText(defaultProps.mainMenuDropdowns[1].items[0].title); + expect(externalLink.getAttribute('href')).toBe(defaultProps.mainMenuDropdowns[1].items[0].href); }); test('renders empty state when there are no dropdowns', () => { - render(); + render(); const mobileMenu = screen.getByTestId('mobile-menu'); expect(mobileMenu).toBeInTheDocument(); diff --git a/src/studio-header/NavDropdownMenu.jsx b/src/studio-header/NavDropdownMenu.jsx index a4fe0eac2..8f46f15e1 100644 --- a/src/studio-header/NavDropdownMenu.jsx +++ b/src/studio-header/NavDropdownMenu.jsx @@ -4,14 +4,12 @@ import { Dropdown, DropdownButton, } from '@openedx/paragon'; - -import { navigateToUrl } from './utils'; +import { Link } from 'react-router-dom'; const NavDropdownMenu = ({ id, buttonTitle, items, - onNavigate, }) => ( {items.map(item => ( navigateToUrl(e, item.href, onNavigate)} + to={item.href} className="small" - href={item.href.startsWith('#') ? item.href : null} > {item.title} @@ -39,7 +37,6 @@ NavDropdownMenu.propTypes = { href: PropTypes.string.isRequired, title: PropTypes.node.isRequired, })).isRequired, - onNavigate: PropTypes.func.isRequired, }; export default NavDropdownMenu; diff --git a/src/studio-header/NavDropdownMenu.test.jsx b/src/studio-header/NavDropdownMenu.test.jsx index ae5409dcf..887c68413 100644 --- a/src/studio-header/NavDropdownMenu.test.jsx +++ b/src/studio-header/NavDropdownMenu.test.jsx @@ -1,9 +1,9 @@ import React from 'react'; import { render, screen, fireEvent } from '@testing-library/react'; import '@testing-library/jest-dom/extend-expect'; -import NavDropdownMenu from './NavDropdownMenu'; +import { MemoryRouter } from 'react-router-dom'; -const mockOnNavigate = jest.fn(); +import NavDropdownMenu from './NavDropdownMenu'; const defaultProps = { id: 'menu-id', @@ -12,9 +12,14 @@ const defaultProps = { { href: '/item1', title: 'Item 1' }, { href: 'https://external.com', title: 'External Link' }, ], - onNavigate: mockOnNavigate, }; +const RootWrapper = (props) => ( + + + +); + describe('NavDropdownMenu Component', () => { afterEach(() => { jest.clearAllMocks(); @@ -23,48 +28,40 @@ describe('NavDropdownMenu Component', () => { test('renders the dropdown button with correct title', () => { render(); - const dropdownButton = screen.getByRole('button', { name: 'Menu' }); + const dropdownButton = screen.getByRole('button', { name: defaultProps.buttonTitle }); expect(dropdownButton).toBeInTheDocument(); }); test('renders all dropdown items', () => { - render(); + render(); - const dropdownButton = screen.getByRole('button', { name: 'Menu' }); + const dropdownButton = screen.getByRole('button', { name: defaultProps.buttonTitle }); fireEvent.click(dropdownButton); - const item1 = screen.getByText('Item 1'); - const externalLink = screen.getByText('External Link'); + const item1 = screen.getByText(defaultProps.items[0].title); + const externalLink = screen.getByText(defaultProps.items[1].title); expect(item1).toBeInTheDocument(); expect(externalLink).toBeInTheDocument(); }); test('calls onNavigate with the correct URL for internal link', () => { - render(); + render(); - const dropdownButton = screen.getByRole('button', { name: 'Menu' }); + const dropdownButton = screen.getByRole('button', { name: defaultProps.buttonTitle }); fireEvent.click(dropdownButton); - const item1 = screen.getByText('Item 1'); - fireEvent.click(item1); - - expect(mockOnNavigate).toHaveBeenCalledWith('/item1'); + const item1 = screen.getByText(defaultProps.items[0].title); + expect(item1.getAttribute('href')).toBe(defaultProps.items[0].href); }); test('navigates to external URL when external link is clicked', () => { - // Mock window.location.href - delete window.location; - window.location = { href: '' }; - - render(); + render(); - const dropdownButton = screen.getByRole('button', { name: 'Menu' }); + const dropdownButton = screen.getByRole('button', { name: defaultProps.buttonTitle }); fireEvent.click(dropdownButton); - const externalLink = screen.getByText('External Link'); - fireEvent.click(externalLink); - - expect(window.location.href).toBe('https://external.com'); + const externalLink = screen.getByText(defaultProps.items[1].title); + expect(externalLink.getAttribute('href')).toBe(defaultProps.items[1].href); }); }); diff --git a/src/studio-header/StudioHeader.jsx b/src/studio-header/StudioHeader.jsx index b9f8808ff..6ad0823b6 100644 --- a/src/studio-header/StudioHeader.jsx +++ b/src/studio-header/StudioHeader.jsx @@ -17,7 +17,7 @@ ensureConfig([ const StudioHeader = ({ number, org, title, containerProps, isHiddenMainMenu, mainMenuDropdowns, - outlineLink, searchButtonAction, onNavigate, isNewHomePage, + outlineLink, searchButtonAction, isNewHomePage, }) => { const { authenticatedUser, config } = useContext(AppContext); const props = { @@ -36,7 +36,6 @@ const StudioHeader = ({ mainMenuDropdowns, outlineLink, searchButtonAction, - onNavigate, }; return ( @@ -68,7 +67,6 @@ StudioHeader.propTypes = { })), outlineLink: PropTypes.string, searchButtonAction: PropTypes.func, - onNavigate: PropTypes.func.isRequired, isNewHomePage: PropTypes.bool.isRequired, }; diff --git a/src/studio-header/StudioHeader.test.jsx b/src/studio-header/StudioHeader.test.jsx index 263cac2f7..793f91ab7 100644 --- a/src/studio-header/StudioHeader.test.jsx +++ b/src/studio-header/StudioHeader.test.jsx @@ -9,6 +9,7 @@ import { import { AppContext } from '@edx/frontend-platform/react'; import { IntlProvider } from '@edx/frontend-platform/i18n'; import { Context as ResponsiveContext } from 'react-responsive'; +import { MemoryRouter } from 'react-router-dom'; import StudioHeader from './StudioHeader'; import messages from './messages'; @@ -40,15 +41,17 @@ const RootWrapper = ({ return ( // eslint-disable-next-line react/jsx-no-constructed-context-values, react/prop-types - - - - - - - + + + + + + + + + ); }; @@ -70,6 +73,7 @@ const props = { ], outlineLink: 'tEsTLInK', searchButtonAction: null, + isNewHomePage: true, }; describe('Header', () => { diff --git a/src/studio-header/UserMenu.jsx b/src/studio-header/UserMenu.jsx index 95a042760..03695c165 100644 --- a/src/studio-header/UserMenu.jsx +++ b/src/studio-header/UserMenu.jsx @@ -14,7 +14,6 @@ const UserMenu = ({ authenticatedUserAvatar, isMobile, isAdmin, - onNavigate, // injected intl, }) => { @@ -39,7 +38,6 @@ const UserMenu = ({ { - e.preventDefault(); - - if (typeof url !== 'string' || !url.trim()) { - return; - } - - const isAbsoluteUrl = /^https?:\/\//i.test(url); - - if (isAbsoluteUrl) { - window.location.href = url; - } else if (typeof onNavigate === 'function') { - onNavigate(url); - } -}; - const getUserMenuItems = ({ studioBaseUrl, logoutUrl,