Skip to content

initial commit#8

Open
nsLittle wants to merge 4 commits into
projectshft:mainfrom
nsLittle:main
Open

initial commit#8
nsLittle wants to merge 4 commits into
projectshft:mainfrom
nsLittle:main

Conversation

@nsLittle

Copy link
Copy Markdown

So ugly, but it works...

Comment thread app/page.js Outdated
useSelector((state) => state.weather);

const handleSearch = () => {
console.log(searchInput);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Remember to clean up console.log in code - should only be used for troubleshooting.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

UgH! I get so excited about submitting the evals because I'm so sick of looking at my wonky apps...

Comment thread app/page.js Outdated

const handleSearch = () => {
console.log(searchInput);
if (searchInput.trim() !=='') {

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: spacing
if (searchInput.trim() !== '') {

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Duly noted.

export const fetchWeather = createAsyncThunk(
'weather/fetchWeatherData',
async (city, thunkAPI) => {
try {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

nice job adding in try catch

'weather/fetchWeatherData',
async (city, thunkAPI) => {
try {
const response = await axios.get(`https://api.openweathermap.org/data/2.5/forecast?q=${city}&appid=1a8c344b0aaddd77f2a53a70916ddc5b&units=imperial`);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

recommend separating out appid as a variable

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I don't understand this?!?

@lmicek lmicek May 20, 2024

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I don't understand this?!?

like this
const appID = '1a8c344b0aaddd77f2a53a70916ddc5b';
&appid=${appID}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I support this comment

@nsLittle

Copy link
Copy Markdown
Author

I made some edits based on Laura's comments. Should be slightly better than last version.

@nsLittle

Copy link
Copy Markdown
Author

Rony/Laura. Hopefully I am submitting the correct version this time. Pretty sure I executed the git commit/push correctly this time. Thank you as always.

Comment thread app/page.js
</a>
dispatch(fetchWeather(searchInput))
.then((response) => {
if (response.payload.cod === '404') {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

interesting that the interface is .cod and not .code

Comment thread app/page.js
</ul>
<ul>
<li className='data'>5-day average: {calculateAverage(result.data.list, 'humidity')}</li>
</ul>

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

this html is a bit long and hard to understand, there's logic also that could have been extracted to functions to help with readability

'weather/fetchWeatherData',
async (city, thunkAPI) => {
try {
const response = await axios.get(`https://api.openweathermap.org/data/2.5/forecast?q=${city}&appid=1a8c344b0aaddd77f2a53a70916ddc5b&units=imperial`);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I support this comment

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.

3 participants