-
Notifications
You must be signed in to change notification settings - Fork 0
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
Stage 4 – Coursework review and evaluation #3
Conversation
…or user on login and register pages.
…nctionality "Add to watchlist" on listing component.
…ting to watchlist. Added new api call for fetching the correct data.
…epending on screen size.
… fields are filled before creating the listing
…age by updating the state of the listing after successful biding
…pi to use accept currency.
…her-improvements # Conflicts: # react-frontend/src/api.js # react-frontend/src/components/Header/MenuIcon/MenuIcon.js
// Use a function to create the axios instance | ||
const createAxiosInstance = () => { | ||
if (process.env.NODE_ENV === 'test') { | ||
// For testing, return the default axios instance |
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.
Could you elaborate on why we need it?
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.
The axios instance was created using
return axios.create({
baseURL: API_BASE_URL,
withCredentials: true,
headers: {
'Content-Type': 'application/json'
}
However, tests do not run and produce the errror:
cannot read properties of undefined (reading 'data') react.
After doing a research I found that default axios instance is needed to run the tests, therefore I decided to add the if condition to create the basic axios instance when running tests else production or development
import './Button.css' | ||
|
||
const Button = ({ type, children, onClick, variant }) => { | ||
const buttonClass = variant === 'transparent' ? 'button transparent' : 'button' |
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.
nit: You can use https://www.npmjs.com/package/classnames
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.
Refactored in b655d78
return ( | ||
<div className="dropdown"> | ||
<a href="/edit-profile">Edit Profile</a> | ||
<a href="/profile">Edit Profile</a> |
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.
Please, use Link
. Regular link reloads the page, but Link
calls the app's rerender without reloading the entire page
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.
Thanks for the idea. Refactored in b655d78
} | ||
|
||
/*.header__dropdown {*/ |
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.
Leftover?
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.
Refactored in b655d78
/* right: 20px;*/ | ||
/*}*/ | ||
|
||
@media (max-width: 768px) { |
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.
It's better to use the opposite approach. Write the main styles for mobile devices and then apply some improvements to the desktop version.
Like here https://github.com/YaroslavKSE/AuctionWeb/pull/3/files#diff-b56c115974c26c9a418c6de5e98389d96c6a74fd7928084c67ec8b3abe878614R97
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.
Refactored in b655d78
fetchListings().catch((error) => console.error('Unhandled error in fetchListings:', error)) | ||
fetchUserWatchlist().catch((error) => | ||
console.error('Unhandled error in fetchUserWatchlist:', error) |
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 add error state
to show the error message in case of any troubles
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.
Added error state in b655d78
setShowAuthAlert(true) | ||
return | ||
} | ||
if (!validateForm()) { |
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.
It would be helpful if we validate each field separately.
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.
Added validation in 1745875
} catch (error) { | ||
console.error('Error fetching listing:', error) | ||
console.error('Error fetching listing or bids:', error) |
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 handle error state
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.
Added error state in this commint: 1745875
if (isAuthenticated) { | ||
try { | ||
const data = await fetchWatchlistIds() | ||
// console.log('Fetched watchlist:', data) |
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.
Letfover?
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.
Yes, refactored 1745875
const data = await fetchUserWatchlist() | ||
setListings(data) | ||
} catch (error) { | ||
console.error('Error fetching watchlist:', error) |
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 add the error state
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.
Okay, refactored in 1745875
expect(result).toEqual(mockResponse.data); | ||
}); | ||
|
||
test('login_ValidCredentials_ReturnsSuccessMessage', async () => { |
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.
In Jest, we can use just human-readable text
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.
I know, but I really prefer naming convention. If it is compulsory I could change all names of the tets. By the way, I have fixed unti tests in 2ca8b01
Cool idea added in cf30232 |
The website is accessible through this link: https://webauction.store/