-
Notifications
You must be signed in to change notification settings - Fork 52
added dates calculator functions, templates and styles #274
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
added dates calculator functions, templates and styles #274
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a great start :)
app/templates/home.html
Outdated
alert("Expected end date"); | ||
return; | ||
} | ||
var date1 = document.getElementById("startDate").value; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use let/const
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
app/templates/home.html
Outdated
@@ -1,21 +1,56 @@ | |||
{% extends "partials/index/index_base.html" %} | |||
|
|||
{% block content %} | |||
<script> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please put the JS in a different file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
Codecov Report
@@ Coverage Diff @@
## develop #274 +/- ##
===========================================
- Coverage 95.15% 95.10% -0.05%
===========================================
Files 76 76
Lines 3382 3352 -30
===========================================
- Hits 3218 3188 -30
Misses 164 164
Continue to review full report at Codecov.
|
…into Features/dates_calculator
app/templates/home.html
Outdated
<label for="endDate">End date:<em>*</em></label> | ||
<input id="endDate" name="date2" type="date"> | ||
</form> | ||
<button type="button" onclick="hiddenDifference()">Calculate</button> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please don't use onclick
- Use addEventListener
instead. (We don't want to mix the logic and the view)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
app/static/js/dates_calculator.js
Outdated
} | ||
const diffTime = Math.abs(date2 - date1); | ||
const diffDays = Math.ceil(diffTime / (1000 * 60 * 60 * 24)); | ||
document.getElementById("demo").innerHTML = "The difference: " + (diffDays) + " days"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Prefer innerText
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
app/static/js/dates_calculator.js
Outdated
date1 = Date.now() | ||
} | ||
const diffTime = Math.abs(date2 - date1); | ||
const diffDays = Math.ceil(diffTime / (1000 * 60 * 60 * 24)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Save as a constant with a proper name
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
app/templates/home.html
Outdated
<input id="endDate" name="date2" type="date"> | ||
</form> | ||
<button type="button" onclick="hiddenDifference()">Calculate</button> | ||
<h3 id="demo"></h3> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the purpose of the h3
? Should it be a title?
If not - prefer using p
/span
/div
tags + CSS
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
app/templates/home.html
Outdated
<div class="container mt-4"> | ||
</div> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the purpose of this div?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesn't exist in the new commit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't see it here. Is there a new PR or something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Every time I pull from the develop this div exists
@@ -5,6 +5,17 @@ | |||
<div class="title">{{ day.display() }}</div> | |||
<div class="sec-title">Location 0<sup>o</sup>c 00:00</div> | |||
</div> | |||
<div style="align: center"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't use inline styles, move them to CSS file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is still here :)
app/templates/home.html
Outdated
<div class="container mt-4"> | ||
</div> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
?
} | ||
const diffDates = Math.abs(date2 - date1); | ||
const diffInDays = Math.ceil(diffDates / (1000 * 60 * 60 * 24)); | ||
document.getElementById("demo").innerText = "The difference: " + (diffInDays) + " days"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can use JS `Template ${strings}`
if you'd like to
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I took this code and I really don't know or want to change something that work pretty code :(
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No problem, but we did talk about taking code from others. Please include full credit in your code.
app/static/js/dates_calculator.js
Outdated
date1 = new Date(date1); | ||
} | ||
else { | ||
date1 = Date.now() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
swal("Expected end date"); | ||
return; | ||
} | ||
let date1 = document.getElementById("startDate").value; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Prefer const
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but date1
changing
app/static/js/dates_calculator.js
Outdated
@@ -0,0 +1,21 @@ | |||
window.onload = function () { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Prefer addEventListener on DOMContentLoaded to prevent the possibility of being overriden
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really don't understand how
} | ||
const diffDates = Math.abs(date2 - date1); | ||
const diffInDays = Math.ceil(diffDates / (1000 * 60 * 60 * 24)); | ||
document.getElementById("demo").innerText = "The difference: " + (diffInDays) + " days"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No problem, but we did talk about taking code from others. Please include full credit in your code.
@@ -5,6 +5,17 @@ | |||
<div class="title">{{ day.display() }}</div> | |||
<div class="sec-title">Location 0<sup>o</sup>c 00:00</div> | |||
</div> | |||
<div style="align: center"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is still here :)
app/templates/home.html
Outdated
<div class="container mt-4"> | ||
</div> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't see it here. Is there a new PR or something?
date1 = Date.now(); | ||
} | ||
const diffDates = Math.abs(date2 - date1); | ||
const diffInDays = Math.ceil(diffDates / (1000 * 60 * 60 * 24)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe set the values to a const?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is const
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I meant the numbers - 1000 * 60 * 60 * 24
, extract it to another const.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is that what matters? It's just a formula
Here you can see the logical in the calculation
window.addEventListener('DOMContentLoaded', (event) => { | ||
document.getElementById("CalcBtn").addEventListener("click", hiddenDifference); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job!
I created a form for two dates. One is for the first date we start from and the second is for the date we end with.
After clicking on the button "calculate" we get the difference in days between the two dates.
We don't have to enter data to the first date input, what will happen is that the first date will become the current date.