-
Notifications
You must be signed in to change notification settings - Fork 3
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Coverage #49
base: main
Are you sure you want to change the base?
Coverage #49
Changes from all commits
1220161
e0bf0e5
b860250
17f7c51
ccb4b2d
3d1c6dc
a723dda
d482ebb
273377c
1d7d071
26a5454
a037629
8bf43ae
374045e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,22 +1,22 @@ | ||
import { FC } from 'react' | ||
import AppBar from '@mui/material/AppBar' | ||
import { FC } from 'react'; | ||
import AppBar from '@mui/material/AppBar'; | ||
import { useTheme } from '@mui/material/styles'; | ||
import useMediaQuery from '@mui/material/useMediaQuery'; | ||
import MobileHeader from './MobileHeader'; | ||
import DesktopHeader from './DesktopHeader'; | ||
|
||
|
||
|
||
const Header: FC = () => { | ||
const theme = useTheme() | ||
let inner = (<DesktopHeader />) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not really sure why we are rewriting this. Prefer the original way There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry, my formatter adds semicolons :orz: will remove these There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Here I'm referring to the refactor from lines 9-20 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The test coverage could not reach |
||
if (useMediaQuery(theme.breakpoints.down('sm'))) { | ||
inner = (<MobileHeader />) | ||
} | ||
const theme = useTheme(); | ||
|
||
return <AppBar position="static" aria-label="header"> | ||
{inner} | ||
</AppBar> | ||
} | ||
return ( | ||
<AppBar position="static" aria-label="header"> | ||
{useMediaQuery(theme.breakpoints.down('sm')) ? ( | ||
<MobileHeader /> | ||
) : ( | ||
<DesktopHeader /> | ||
)} | ||
</AppBar> | ||
); | ||
}; | ||
|
||
export default Header | ||
export default Header; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,14 +1,26 @@ | ||
import { describe, expect, it } from 'vitest' | ||
import { render } from '@/tests/customRender' | ||
import { screen } from '@testing-library/react' | ||
import Header from '../Header' | ||
import { Mock } from 'vitest'; | ||
import useMediaQuery from '@mui/material/useMediaQuery'; | ||
import { screen } from '@testing-library/react'; | ||
import { render } from '@/tests/customRender'; | ||
import Header from '../Header'; | ||
|
||
vi.mock('@mui/material/useMediaQuery'); | ||
|
||
describe('Header', () => { | ||
it('should display the Header in desktop mode on wide screens', async () => { | ||
render(<Header />) | ||
const title = await screen.findByText('WiSE Japan') | ||
expect(title).toBeVisible() | ||
const toolbar = await screen.findByLabelText('desktop-toolbar') | ||
expect(toolbar).toBeVisible() | ||
}) | ||
}) | ||
render(<Header />); | ||
const title = await screen.findByText('WiSE Japan'); | ||
expect(title).toBeVisible(); | ||
const toolbar = await screen.findByLabelText('desktop-toolbar'); | ||
expect(toolbar).toBeVisible(); | ||
}); | ||
|
||
it('should display the Header in mobile mode on smaller screens', async () => { | ||
(useMediaQuery as Mock).mockReturnValueOnce(true); | ||
render(<Header />); | ||
const title = await screen.findByText('WiSE Japan'); | ||
expect(title).toBeVisible(); | ||
const toolbar = await screen.findByLabelText('mobile-toolbar'); | ||
expect(toolbar).toBeVisible(); | ||
}); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,13 @@ | ||
import { screen } from '@testing-library/react'; | ||
import { render } from '@/tests/customRender'; | ||
import ImageCard from '../ImageCard'; | ||
|
||
describe('ImageCard', () => { | ||
it('should render ImageCard component correctly', async () => { | ||
render( | ||
<ImageCard src="https://i.imgur.com/pEI5qWM.jpeg" alt="Cat 🐱" /> | ||
); | ||
const image = await screen.findByRole('img'); | ||
expect(image).toBeVisible(); | ||
}); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,44 +1,33 @@ | ||
import { FC, useEffect, useState, useCallback } from 'react' | ||
import ToggleButton from '@mui/material/ToggleButton' | ||
import ToggleButtonGroup from '@mui/material/ToggleButtonGroup' | ||
import { FC, useEffect, useState } from 'react'; | ||
import { useTranslation } from 'react-i18next'; | ||
import Locale from '@/i18n/locales' | ||
import ToggleButton from '@mui/material/ToggleButton'; | ||
import ToggleButtonGroup from '@mui/material/ToggleButtonGroup'; | ||
import Locale from '@/i18n/locales'; | ||
|
||
const LocaleToggle: FC = () => { | ||
const [locale, setLocale] = useState(Locale.EN); | ||
const { i18n } = useTranslation(); | ||
|
||
const changeLanguage = useCallback(async (nextLocale: Locale) => { | ||
await i18n.changeLanguage(nextLocale).then(() => { | ||
setLocale(nextLocale); | ||
localStorage.setItem('locale', nextLocale); | ||
}); | ||
}, [i18n]); | ||
const [locale, setLocale] = useState( | ||
() => localStorage.getItem('locale') || Locale.EN | ||
); | ||
|
||
useEffect(() => { | ||
const savedLocale = localStorage.getItem('locale'); | ||
if (savedLocale) { | ||
changeLanguage(savedLocale as Locale).catch((e: Error) => { | ||
console.error(e) | ||
}) | ||
} | ||
}, [changeLanguage]); | ||
i18n.changeLanguage(locale).catch(console.error); | ||
}, [locale, i18n]); | ||
|
||
const handleChange = (_: React.MouseEvent<HTMLElement>, nextLocale: Locale) => { | ||
changeLanguage(nextLocale).catch((e: Error) => { | ||
console.error(e) | ||
}) | ||
} | ||
const handleChange = ( | ||
_: React.MouseEvent<HTMLElement>, | ||
nextLocale: Locale | ||
) => { | ||
setLocale(nextLocale); | ||
localStorage.setItem('locale', nextLocale); | ||
}; | ||
|
||
return ( | ||
<ToggleButtonGroup | ||
exclusive | ||
value={locale} | ||
onChange={handleChange}> | ||
<ToggleButtonGroup exclusive value={locale} onChange={handleChange}> | ||
<ToggleButton value={Locale.EN}>English</ToggleButton> | ||
<ToggleButton value={Locale.JA}>日本語</ToggleButton> | ||
</ToggleButtonGroup> | ||
); | ||
} | ||
}; | ||
|
||
export default LocaleToggle | ||
export default LocaleToggle; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,27 +1,66 @@ | ||
import { describe, expect, it } from 'vitest' | ||
import { render } from '@/tests/customRender' | ||
import { screen } from '@testing-library/react'; | ||
import { Mock } from 'vitest'; | ||
import { useTranslation } from 'react-i18next'; | ||
|
||
import { render } from '@/tests/customRender'; | ||
import LocaleToggle from '../LocaleToggle'; | ||
import userEvent from '@testing-library/user-event' | ||
import { screen } from '@testing-library/react' | ||
|
||
vi.mock('react-i18next', async (importOriginal) => { | ||
const actual = await importOriginal<typeof import('react-i18next')>(); | ||
return { | ||
...actual, | ||
useTranslation: vi.fn(), | ||
}; | ||
}); | ||
|
||
const useTranslationSpy = useTranslation as Mock; | ||
|
||
beforeEach(() => { | ||
vi.clearAllMocks(); | ||
|
||
useTranslationSpy.mockReturnValue({ | ||
i18n: { changeLanguage: vi.fn(() => new Promise(() => {})) }, | ||
}); | ||
}); | ||
|
||
describe('LocaleToggle', () => { | ||
it('renders correctly', async () => { | ||
render(<LocaleToggle />) | ||
render(<LocaleToggle />); | ||
|
||
const english = await screen.findByText('English'); | ||
const japanese = await screen.findByText('日本語'); | ||
expect(english).toBeInTheDocument(); | ||
expect(japanese).toBeInTheDocument(); | ||
}); | ||
}) | ||
|
||
it('changes locale when toggling', async () => { | ||
render(<LocaleToggle />); | ||
const japaneseButton = screen.getByText('日本語'); | ||
it('changes locale when toggling', async () => { | ||
const { user } = render(<LocaleToggle />); | ||
const japaneseButton = screen.getByRole('button', { name: '日本語' }); | ||
|
||
const user = userEvent.setup() | ||
await user.click(japaneseButton) | ||
expect(localStorage.getItem('locale')).toBe('ja'); | ||
await user.click(japaneseButton); | ||
expect(localStorage.getItem('locale')).toBe('ja'); | ||
|
||
localStorage.removeItem('locale'); | ||
localStorage.removeItem('locale'); | ||
}); | ||
|
||
it('handles failed changeLanguage error correctly', async () => { | ||
// Mock changeLanguage function to throw an error | ||
useTranslationSpy.mockReturnValue({ | ||
i18n: { changeLanguage: vi.fn().mockRejectedValue(new Error()) }, | ||
}); | ||
|
||
const consoleErrorSpy = vi | ||
.spyOn(console, 'error') | ||
.mockImplementation(() => {}); | ||
|
||
const { user } = render(<LocaleToggle />); | ||
const japaneseButton = screen.getByRole('button', { name: '日本語' }); | ||
|
||
await user.click(japaneseButton); | ||
// `console.error` should be called with an error | ||
expect(consoleErrorSpy).toHaveBeenCalledWith(expect.any(Error)); | ||
|
||
consoleErrorSpy.mockRestore(); | ||
useTranslationSpy.mockRestore(); | ||
}); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,36 +1,38 @@ | ||
import { FC } from 'react' | ||
import Box from '@mui/material/Box' | ||
import Divider from '@mui/material/Divider' | ||
import List from '@mui/material/List' | ||
import ListItem from '@mui/material/ListItem' | ||
import { useTheme } from '@mui/material/styles' | ||
import useMediaQuery from '@mui/material/useMediaQuery' | ||
import StyledNavLink from '../StyledNavLink/StyledNavLink' | ||
import LocaleToggle from '../LocaleToggle/LocaleToggle' | ||
import { FC } from 'react'; | ||
import Box from '@mui/material/Box'; | ||
import Divider from '@mui/material/Divider'; | ||
import List from '@mui/material/List'; | ||
import ListItem from '@mui/material/ListItem'; | ||
import { useTheme } from '@mui/material/styles'; | ||
import useMediaQuery from '@mui/material/useMediaQuery'; | ||
import StyledNavLink from '../StyledNavLink/StyledNavLink'; | ||
import LocaleToggle from '../LocaleToggle/LocaleToggle'; | ||
|
||
const DrawerContents: FC = () => { | ||
const theme = useTheme() | ||
let navList = <></> | ||
if (useMediaQuery(theme.breakpoints.down('sm'))) { | ||
navList = (<> | ||
<ListItem> | ||
<StyledNavLink to='/'>Home</StyledNavLink> | ||
</ListItem> | ||
<ListItem> | ||
<StyledNavLink to='/codeofconduct'>Code of Conduct</StyledNavLink> | ||
</ListItem> | ||
<Divider /> | ||
</>) | ||
} | ||
const theme = useTheme(); | ||
|
||
return <Box sx={{ width: 300 }}> | ||
<List> | ||
{navList} | ||
<ListItem> | ||
<LocaleToggle /> | ||
</ListItem> | ||
</List> | ||
</Box> | ||
} | ||
return ( | ||
<Box sx={{ width: 300 }}> | ||
<List> | ||
{useMediaQuery(theme.breakpoints.down('sm')) && ( | ||
<> | ||
<ListItem> | ||
<StyledNavLink to="/">Home</StyledNavLink> | ||
</ListItem> | ||
<ListItem> | ||
<StyledNavLink to="/codeofconduct"> | ||
Code of Conduct | ||
</StyledNavLink> | ||
</ListItem> | ||
<Divider /> | ||
</> | ||
)} | ||
<ListItem> | ||
<LocaleToggle /> | ||
</ListItem> | ||
</List> | ||
</Box> | ||
); | ||
}; | ||
|
||
export default DrawerContents | ||
export default DrawerContents; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's separate stylistic changes from logical changes to make the review size smaller.
I noticed recently there is inconsistent semicolon usage. We can talk about whether we want the semi rule on or off, I've heard good arguments for both cases.