-
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
Merge Dev Branch #6
Conversation
Project Setup
Fetch and Aisplay Data
change http to https
Add live version link to README.md file
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.
Hi @momo-87,
Great job so far! 👏
You've done well, however, there are some issues that you still need to work on to go to the next project, but you are almost there!
To highlight 🎯
- No linter errors.✔️
- Nice design. ✔️
- Dice project implementation. ✔️
- Good commit messages ✔️
Required Changes ♻️
Check the comments under the review.
Optional suggestions
Check the comments under the review.
Every comment with the [OPTIONAL] prefix won't stop the approval of this PR. However, I strongly recommend you to take them into account as they can make your code better. Some of them were simply missed by the previous reviewer and addressing them will really improve your application.
Cheers, and Happy coding!👏👏👏
Feel free to leave any questions or comments in the PR thread if something is not 100% clear.
Please, remember to tag me in your question so I can receive the notification.
Please, do not open a new Pull Request for re-reviews. You should use the same Pull Request submitted for the first review, either valid or invalid unless it is requested otherwise.
As described in the Code reviews limits policy you have a limited number of reviews per project (check the exact number in your Dashboard). If you think that the code review was not fair, you can request a second opinion using this form.
src/styles/navbar.scss
Outdated
// ul { | ||
// display: flex; | ||
// height: 60px; | ||
|
||
// li a { | ||
// color: #F1F6F9; | ||
// } | ||
// } |
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, kindly remove these commented pieces of code to make your codebase clean and maintainable
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.
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.
Thank you, I really appreciate the time you spend reviewing my project.
Gill Sans and Lato font added to the project.
src/components/OtherCitiesList.jsx
Outdated
import { addClickedCity } from 'redux/home/homeSlice'; | ||
|
||
const OtherCitiesList = ({ otherCities, isLoading, error }) => { | ||
// const { otherCities, isLoading, error } = useSelector(getData); |
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, kindly remove these commented pieces of code to make your codebase clean and maintainable
} | ||
); | ||
} catch (error) { | ||
return rejectWithValue('Your Location is not supported !!'); |
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.
[OPTIONAL]
- Hey, at the start of your page, it throws the error for some specific country, e.g Nigeria, the app would throw that error, The app should be accessible to any country of the world and as such, please, kindly improve on this to make the app accessible to any country to improve the user experience of your app.
} | ||
); | ||
} catch (error) { | ||
return rejectWithValue('Your Location is not supported !!'); |
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.
[OPTIONAL]
- Hey, at the start of your page, it throws the error for some specific country, e.g Nigeria, the app would throw that error, The app should be accessible to any country of the world and as such, please, kindly improve on this to make the app accessible to any country to improve the user experience of your app.
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.
Unfortunately, the API provides the data so, I can manage this aspect. Thank you for your observation and feedback.
## 🚀 Live Demo <a name="live-demo"></a> | ||
>[Live Demo](https://airmetrics.onrender.com) |
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.
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.
Thank you, I really appreciate the time you spend reviewing my project.
The video presentation link was added to the readme file.
import '@testing-library/jest-dom'; | ||
import React from 'react'; | ||
import { render } from '@testing-library/react'; | ||
import { MemoryRouter } from 'react-router-dom'; | ||
import { Provider } from 'react-redux'; | ||
import { configureStore } from '@reduxjs/toolkit'; | ||
import data from './data'; | ||
import App from '../App'; | ||
|
||
const reducer = ( | ||
state = { | ||
home: { home: data }, | ||
}, | ||
) => state; | ||
|
||
const store = configureStore({ reducer }); | ||
describe('App test', () => { | ||
it('will test App rendering', () => { | ||
render( | ||
<MemoryRouter> | ||
<Provider store={store}> | ||
<App /> | ||
</Provider> | ||
</MemoryRouter>, | ||
); | ||
}); | ||
}); |
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.
-
According to the project requirements :
- The App should have unit tests for several components of the app and some pure functions in Redux code (currently you only tested the navbar component and App). The App should have integration tests using React Testing Library. Please, kindly add that 🙏
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.
Thank you, I really appreciate the time you spend reviewing my project.
I added two additional tests for SelectedCity and DetailsPages respectively.
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.
STATUS: CHANGES REQUIRED ♻️♻️
Hi there @momo-87 👋🏽 ,
Great work on making the changes requested by a previous reviewer 👏
To highlight 👍🏽
✔️ Tests are added
✔️ Tests pass
✔️ App runs as expected
You've done well implementing some of the requested changes, but there are still some which aren't addressed yet.
Required Changes ♻️
Check the comments under the review.
Optional suggestions
Every comment with the [OPTIONAL] prefix won't stop the approval of this PR. However, I strongly recommend you to take them into account as they can make your code better. Some of them were simply missed by the previous reviewer and addressing them will really improve your application.
Cheers and Happy coding!👏👏👏
Feel free to leave any questions or comments in the PR thread if something is not 100% clear.
Please, remember to tag me in your question so I can receive the notification.
Please, do not open a new Pull Request for re-reviews. You should use the same Pull Request submitted for the first review, either valid or invalid unless it is requested otherwise.
As described in the Code reviews limits policy you have a limited number of reviews per project (check the exact number in your Dashboard). If you think that the code review was not fair, you can request a second opinion using this form.
src/tests/NearestCity.test.js
Outdated
import store from 'redux/store'; | ||
import NearestCity from 'components/NearestCity'; | ||
|
||
// eslint-disable-next-line react/prop-types |
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.
- Kindly do not disable linters as it helps us write cleaner code
const store = configureStore({ reducer }); | ||
describe('App test', () => { | ||
it('will test App rendering', () => { | ||
render( | ||
<MemoryRouter> | ||
<Provider store={store}> | ||
<App /> | ||
</Provider> | ||
</MemoryRouter>, | ||
); |
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.
README.md
Outdated
|
||
## 🚀 Live Demo <a name="live-demo"></a> | ||
>[Live Demo](https://airmetrics.onrender.com) | ||
> [Video Presentation](https://www.loom.com/share/7d841e25e3ca4efd983da5fa5d90c2c2?sid=85dd4d9f-2734-4d6c-9233-d8c6a07ec87d) |
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.
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.
Hi @momo-87,
In my opinion, your project is ready for the presentation! There is nothing else to say other than... it's time to merge it
Good luck! 🎉
Feel free to leave any questions or comments in the PR thread if something is not 100% clear.
Merge pull request #6 from momo-87/dev
Added Features: