Skip to content

weather app#5

Open
JwThomas21 wants to merge 5 commits into
projectshft:mainfrom
JwThomas21:main
Open

weather app#5
JwThomas21 wants to merge 5 commits into
projectshft:mainfrom
JwThomas21:main

Conversation

@JwThomas21

Copy link
Copy Markdown

No description provided.

Comment thread components/Chart.js
import { Sparklines, SparklinesLine } from 'react-sparklines';

const Chart = ({ data, title }) => {
console.log('Chart Component Props - Data:', data);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

chore: clean up console.log

Comment thread hooks/useWeather.js
@@ -0,0 +1,10 @@
import { useSelector, useDispatch } from 'react-redux';

const useWeather = () => {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

praise: very clean looking, great job!

Comment thread pages/index.js
const [city, setCity] = useState('');
const { state, dispatch } = useWeather();

const handleSubmit = async (e) => {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpick: avoid vague names like e - would recommend event instead here

loading: false,
error: action.payload
};
default:

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

praise: great job covering the default case

Comment thread redux/store.js
const store = configureStore({
reducer: {
weather: weatherReducer,
},

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpick: remove hanging commas when there are no values after them

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants