Skip to content

RTK Weather - Mark Smyth#15

Open
XTruckDriver wants to merge 14 commits intoprojectshft:mainfrom
XTruckDriver:main
Open

RTK Weather - Mark Smyth#15
XTruckDriver wants to merge 14 commits intoprojectshft:mainfrom
XTruckDriver:main

Conversation

@XTruckDriver
Copy link
Copy Markdown

No description provided.

Comment on lines +25 to +26
let sum = array.reduce((a, b) => a + b, 0);
let avg = (sum / array.length);
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

should be const, not let

Comment on lines +16 to +18



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 the code spacing and alignment



cities.forEach((city, index) => {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

no need for empty lines like these

Comment on lines +43 to +47
cities?.map((city, index) => (

<CityForecast key={index} city={city} index={index} />

))
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

code alignment is off here and makes the code review difficult

</div>

{
!Array.isArray(cities) || cities.length === 0 && (
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

could have made this a function that takes cities for argument and would make the code more readable.

Comment on lines +11 to +13
// store.subscribe(() => {
// console.log("Store State: ", store.getState());
// });
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

do not submit commented code

Comment on lines +44 to +64
<li className='list-group-item col-3'>
<Sparklines limit={40} width={200} height={100} data={tempArray} >
<SparklinesLine color="#40c0f5" />
<SparklinesReferenceLine type="avg" />
</Sparklines>
<span>{computeAverage(tempArray)} F</span>
</li>
<li className='list-group-item col-3'>
<Sparklines limit={40} width={200} height={100} data={pressureArray} >
<SparklinesLine color="#d1192e" />
<SparklinesReferenceLine type="avg" />
</Sparklines>
<span>{computeAverage(pressureArray)} hPa</span>
</li>
<li className='list-group-item col-3'>
<Sparklines limit={40} width={200} height={100} data={humidityArray} >
<SparklinesLine color="#8ed53f" />
<SparklinesReferenceLine type="avg" />
</Sparklines>
<span>{computeAverage(humidityArray)}%</span>
</li>
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, this could have been a loop

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