Skip to content

Redux Eval#19

Open
acl13 wants to merge 13 commits into
projectshft:mainfrom
acl13:main
Open

Redux Eval#19
acl13 wants to merge 13 commits into
projectshft:mainfrom
acl13:main

Conversation

@acl13

@acl13 acl13 commented Jan 24, 2025

Copy link
Copy Markdown

No description provided.

Comment on lines +3 to +7
<div className="row p-3 border-top">
<div className="col-sm text-center fw-bold">City</div>
<div className="col-sm text-center fw-bold">Temperature (F)</div>
<div className="col-sm text-center fw-bold">Pressure (hPa)</div>
<div className="col-sm text-center fw-bold">Humidity (%)</div>

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

dry, you can use .map to do this more efficiently

Comment on lines +5 to +11
<div className="col-sm">
<Sparklines data={data}>
<SparklinesCurve color={color} />
<SparklinesReferenceLine type="avg"></SparklinesReferenceLine>
</Sparklines>
<p className="text-center">{text}</p>
</div>

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

check your code alignment

Comment thread app/page.js
const onSearch = async () => {
//Prevents user from submitting an empty search
if (city.trim() === "") {
alert("Please enter a valid city");

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

alerting is not a great UX, consider displaying the error properly

Comment thread app/page.js
alert(
"Oops, something went wrong. Please check that you have entered a valid city and try again."
);
setCity("");

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

dry, create a function that handles this, will with readability

Comment thread app/page.js
></SearchBar>
<HeadingsRow />
{data &&
cityWeatherData.map((city) => (

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

love this

Comment thread app/page.js
Comment on lines +98 to +108
<WeatherDataChart
data={city.temperatureData}
color={"orange"}
text={city.temperatureAverage}
/>
<WeatherDataChart
data={city.pressureData}
color={"green"}
text={city.pressureAverage}
/>
<WeatherDataChart

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 could have done another map here.

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