Skip to content

Ben Doggett Redux Eval#20

Open
bhdoggett wants to merge 17 commits into
projectshft:mainfrom
bhdoggett:main
Open

Ben Doggett Redux Eval#20
bhdoggett wants to merge 17 commits into
projectshft:mainfrom
bhdoggett:main

Conversation

@bhdoggett

Copy link
Copy Markdown

No description provided.

Comment thread .env Outdated
@@ -0,0 +1 @@
NEXT_PUBLIC_API_KEY="6427275c4ee8b157888fdf144b2fc5ca" No newline at end of file

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

you generally want to add .env files to .gitignore, so it's not visible on the repo, where anyone can use it.

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.

Doh, I was proud of myself for putting it in the .env but forgot to add to .gitignore. Fixed now.

Comment thread app/page.tsx Outdated
import { indexData } from "./types/indexData";
import type { cityProcessedData } from "./types/cityProcessedData";

type weatherPropsType = {

@greypants greypants Jan 29, 2025

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

It's convention to TitleCase type names. Also to prefer interface over type where possible

Comment thread app/page.tsx Outdated
>
<div className="font-medium text-sm my-auto">{city.city}</div>
{weatherCharts(city).map((props) => (
<Chart {...props} key={props.avg} />

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

they key needs to be guaranteed to be unique. If your data had two items with the same .avg value, you'd get a duplicate key warning or it might not even render one of them, I forget. You could use color instead, since you control that property and know it's always unique. Or it would be safe to use index from map as well in this instance. weatherCharts(city).map((props, index) => (

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.

Since avg is actually a string with a unit attached (eg. "72 degrees", "84 %" I thought it would be safe?

})
.addCase(fetchWeather.fulfilled, (state, action) => {
state.status = "succeeded";
state.cities = [...state.cities, action.payload];

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

state.cities.push(action.payload); instead

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