Feat/mini challenge 5#108
Conversation
| clearInterval(intervalId); | ||
| page.removeEventListener('click', rotateBackground); | ||
| }; | ||
| }, []); |
There was a problem hiding this comment.
useLayoutEffect(() => {
const page = mainPage.current;
function rotateBackground() {
const xPercent = random(100);
const yPercent = random(100);
page.style.setProperty('--bg-position', `${xPercent}% ${yPercent}%`);
}
const intervalId = setInterval(rotateBackground, 3000);
page.addEventListener('click', rotateBackground);
return () => {
clearInterval(intervalId);
page.removeEventListener('click', rotateBackground);
};
}, []);Good logic, it's ok. Try to abstract this as a way of separate this logic from the component
| ); | ||
| expect(layoutMain.find('.background-dark').exists()).toBe(true); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
It's ok, but why enzyme and not testing library?
This should be done with testing library.
| handleClose(); | ||
| }; | ||
| const handleSubmitSession = () => { | ||
| const loginResult = loginApi(username, password); |
There was a problem hiding this comment.
const loginResult = loginApi(username, password);
loginResult
.then((data) => {
setErrorSession(false);
setSessionData(data.id, data.username);
handleClose();
})
.catch(() => {
setErrorSession(true);
});It's ok. You can use async await in order to resolve the promise as well inside of a try catch block, and the code will be more readable
| const username = screen.getByText(state.sessionData.username); | ||
| expect(username).toBeInTheDocument(); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
It's not recommended to mix enzyme and testing library utilities on the same test. It could works, but for a developer that is not familiarized with the code, there will be some "wtf" expressions (difficult to understand what is happend)
| /> | ||
| </BrowserRouter> | ||
| </AppProvider> | ||
| ); |
There was a problem hiding this comment.
wrapper = render(
<AppProvider>
<BrowserRouter>
<RelatedVideo
currentVideoId="nmXMgqjQzls"
videos={{ items: videoData }}
path="video"
/>
</BrowserRouter>
</AppProvider>
);If you are duplicating this code (the Providers and Router wrappers) in other tests, try to figure it out how to encapsulate or abstract that logic and reuse it
| const thumbnail = screen.getByRole('img'); | ||
| expect(thumbnail).toHaveAttribute('alt', 'page not found'); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Check this way of test the alt value: https://github.com/testing-library/jest-dom#tohaveaccessiblename
Acceptance Criteria
Bonus Points
|
No description provided.