Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cedar - Rebeca Muniz #76

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Cedar - Rebeca Muniz #76

wants to merge 4 commits into from

Conversation

rebecamuniz
Copy link

No description provided.

@rebecamuniz rebecamuniz reopened this Dec 13, 2021
Copy link

@beccaelenzil beccaelenzil left a comment

Choose a reason for hiding this comment

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

Great work on this Vanilla javascript project. You've met the learning goals around writing semantic html, dynamically applying styles, and event handling using vanilla javascript. Nice work!


<section class = "temperature-side-container" action="#">
<h3> Temperature </h3>
<p id = "temperature-value"> 76 </p>

Choose a reason for hiding this comment

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

Minor note: it is a convention not to include spaces between a properties and it's value.

Suggested change
<p id = "temperature-value"> 76 </p>
<p id="temperature-value"> 76 </p>

<title>Document</title>
<link rel="stylesheet" href= "styles/index.css"/>
</head>
<body class="parent-side-container">

Choose a reason for hiding this comment

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

Nice work writing clear, semantic HTML!


<section class = "temperature-side-container" action="#">
<h3> Temperature </h3>
<p id = "temperature-value"> 76 </p>

Choose a reason for hiding this comment

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

Consider initializing with class name 'orange' so when the page first loads the 76 is orange

}

const changeTempTextColor = () => {
if(state.tempCount >= 80) {

Choose a reason for hiding this comment

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

A chained conditional if\else if\else if would make this code a bit more efficient. Also, minor note, if orangize it as temp <= 49 / else if temp <= 59 ... we do not need the compound conditional.

snowy: "🌨❄️🌨🌨❄️❄️🌨❄️🌨❄️❄️🌨",
}

const state = {

Choose a reason for hiding this comment

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

Nice use of state object

cityInput = cityInput.defaultValue
}

const registerEventHandlers = (event) => {

Choose a reason for hiding this comment

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

Great work organizing this event listeners in the registerEventHandlers function. You might consider refactoring grabbing the corresponding elements that are declared as variables at the top of the script here 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.

2 participants