-
Notifications
You must be signed in to change notification settings - Fork 52
feat: daily horoscope by sign #151
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
Conversation
Great job on the JS part! You can research about |
Codecov Report
@@ Coverage Diff @@
## develop #151 +/- ##
===========================================
+ Coverage 99.21% 99.38% +0.16%
===========================================
Files 39 37 -2
Lines 1530 1457 -73
===========================================
- Hits 1518 1448 -70
+ Misses 12 9 -3
Continue to review full report at Codecov.
|
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 work! Please see my insights regarding the JS part :)
app/static/horoscope.js
Outdated
xhr.send(); | ||
} | ||
|
||
var elements = document.getElementsByClassName('sign'); |
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 using const
and putting this inside the addEventLoop
function
app/static/horoscope.js
Outdated
|
||
var elements = document.getElementsByClassName('sign'); | ||
|
||
function addEventsLoop() { |
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 to name this function better
app/static/horoscope.js
Outdated
@@ -0,0 +1,26 @@ | |||
async function daily_horoscope(singName) { |
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 to name this function better. Also, it doesn't have to be an async function
app/static/horoscope.js
Outdated
xhr.onload = function() { | ||
let obj = JSON.parse(this.responseText); | ||
let daily = document.getElementById('daily_horoscope'); | ||
let str = obj.description; |
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.
Better naming
Great job :) Please increase coverage in events.py and find better names for the JS variables :) |
https://forums.pythonic.guru/t/topic/8790