Skip to content

Master#1

Open
MohammedAlabd wants to merge 2 commits intocomparefrom
master
Open

Master#1
MohammedAlabd wants to merge 2 commits intocomparefrom
master

Conversation

@MohammedAlabd
Copy link
Collaborator

checking on the code

"@testing-library/jest-dom": "^5.12.0",
"@testing-library/react": "^11.2.7",
"@testing-library/user-event": "^12.8.3",
"city": "^1.0.4",
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

remove any dependency that you are not using

there something dependencies hell, try to read

Comment on lines +7 to +9



Copy link
Collaborator Author

Choose a reason for hiding this comment

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

remove those extra lines

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

there an extension called prettier, try to use

Comment on lines +3 to +5
// import Weather from './component/Weather';
//import Weatherrr from './component/Weatherrr';
// import Wether from './component/Wether';
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

don't leave commented code in your prod

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

here and everywhere in your code

import React, { useState } from 'react'
import Time from './Time'
import Weatherrr from './Weatherrr'
const weather = require('openweather-apis');
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

can't you use import here?

Suggested change
const weather = require('openweather-apis');
import weather from 'openweather-apis';

const weather = require('openweather-apis');

function Inputcity() {
const [cooord, setcooord] = useState({ lati: '51.5085', long: '-0.1257', cityy: 'London' })
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

writing it like cooord is so confusing
try to call it something like weatherCoord

alert('Input a valid City')
}
else {
setcooord({ lati: info.coord.lat, long: info.coord.lon, cityy: info.name })
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

the city here is hard coded, you should make it variable
it's also bad named, try to use a different name


function Inputcity() {
const [cooord, setcooord] = useState({ lati: '51.5085', long: '-0.1257', cityy: 'London' })
const [city, setcity] = useState('London')
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

you have a typo here

Suggested change
const [city, setcity] = useState('London')
const [city, setCity] = useState('London')

and maybe you can make it like this

Suggested change
const [city, setcity] = useState('London')
const [cityInput, setCityInput] = useState('London')

Comment on lines +37 to +39
<input type='text' value={city} onChange={cityhandler} autoFocus />
<button onClick={submit}>OK</button>
<button onClick={loctate}>Loctate My Location</button>
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

you have to activate the submit on enter

import moment from 'moment'

function Time() {
const [time, settime] = useState(moment().format('MMMM Do YYYY, h:mm:ss a'))
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Suggested change
const [time, settime] = useState(moment().format('MMMM Do YYYY, h:mm:ss a'))
const [time, setTime] = useState(moment().format('MMMM Do YYYY, h:mm:ss a'))

Comment on lines +8 to +9
lat: `${props.lat}`, //lat
lon: `${props.long}`, //lon
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Suggested change
lat: `${props.lat}`, //lat
lon: `${props.long}`, //lon
lat: props.lat, //lat
lon: props.long, //lon

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