Skip to content

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

Merged
merged 16 commits into from
Feb 7, 2021

Conversation

nadav-pesach
Copy link
Contributor

@yammesicka
Copy link
Member

Great job on the JS part! You can research about fetch if you want to even further improve it.
I've left few extra notes - please make sure your commit doesn't harm the HTML of previous commits.

@codecov-io
Copy link

codecov-io commented Feb 1, 2021

Codecov Report

Merging #151 (28adc74) into develop (6e6a388) will increase coverage by 0.16%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             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     
Impacted Files Coverage Δ
app/routers/profile.py 100.00% <100.00%> (ø)
app/database/models.py 96.22% <0.00%> (-0.24%) ⬇️
app/internal/astronomy.py
app/internal/on_this_day_events.py

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6e6a388...c2cfad2. Read the comment docs.

Copy link
Member

@yammesicka yammesicka 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! Please see my insights regarding the JS part :)

xhr.send();
}

var elements = document.getElementsByClassName('sign');
Copy link
Member

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


var elements = document.getElementsByClassName('sign');

function addEventsLoop() {
Copy link
Member

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

@@ -0,0 +1,26 @@
async function daily_horoscope(singName) {
Copy link
Member

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

xhr.onload = function() {
let obj = JSON.parse(this.responseText);
let daily = document.getElementById('daily_horoscope');
let str = obj.description;
Copy link
Member

Choose a reason for hiding this comment

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

Better naming

@yammesicka
Copy link
Member

Great job :) Please increase coverage in events.py and find better names for the JS variables :)

@yammesicka yammesicka merged commit 152140e into PythonFreeCourse:develop Feb 7, 2021
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.

4 participants