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.

Sign up for GitHub

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

Jump to bottom

Fix issue#29 and 31 #41

Merged
merged 7 commits into from
Oct 6, 2020
Merged

Fix issue#29 and 31 #41

merged 7 commits into from
Oct 6, 2020

Conversation

ry-v1
Copy link
Contributor

@ry-v1 ry-v1 commented Oct 4, 2020

@toonarmycaptain - I have fixed issues #29 and #31. Please review

This was referenced Oct 4, 2020
Closed
Open
@codecov
Copy link

codecov bot commented Oct 4, 2020 •
edited
Loading

Codecov Report

Merging #41 into development will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff              @@
##           development      #41   +/-   ##
============================================
  Coverage        99.57%   99.57%           
============================================
  Files               19       19           
  Lines              237      237           
============================================
  Hits               236      236           
  Misses               1        1

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 6dccdf7...ce0b928. Read the comment docs.

@toonarmycaptain toonarmycaptain added the hacktoberfest-accepted Accept PR, merge or reject later. label Oct 5, 2020
toonarmycaptain
toonarmycaptain reviewed Oct 5, 2020
View reviewed changes
Copy link
Owner

@toonarmycaptain toonarmycaptain left a comment •
edited
Loading

Choose a reason for hiding this comment

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

This looks good. All it needs is to fix the indentation that CI is complaining about. Thankyou!

I'm going to leave the issue open with more emphasis on prettifying the site.

NB If you look at Travis, it's only failing because your copy of the repo doesn't have the key needed to send coverage metrics to Codacy, all the tests are actually passing. :)

probable_immunity_web_app/templates/immunity_app/immunity_results.html Outdated
@@ -14,7 +14,7 @@ <h1>{% block title %}Immunity results:{% endblock %}</h1>

{% endfor %}.

<p align="right">
<p class="back">
Copy link
Owner

Choose a reason for hiding this comment

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

I'll probably rename that class, but I can't think of anything better right now!

toonarmycaptain
toonarmycaptain requested changes Oct 5, 2020
View reviewed changes
Copy link
Owner

@toonarmycaptain toonarmycaptain left a comment

Choose a reason for hiding this comment

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

This looks good, if you can fix the indentation in style.css that would be great :) thanks

probable_immunity_web_app/static/style.css Outdated
@@ -0,0 +1,3 @@
p.back {
text-align: right;
Copy link
Owner

Choose a reason for hiding this comment

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

If you look at the CI checks, Codacy is complaining that the indentation here is 4 spaces instead of two (which is the convention AFAICT), can you fix that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure. I will do.

@toonarmycaptain
Copy link
Owner

Looks good, cheers 👍

In future, it's helpful to make your commit messages more descriptive than Update *file* so it's easy to look at what changes were made where.

I've made some edits, removing some of the html tags that really are overcomplicating things right now, and improved the indentation.

@toonarmycaptain toonarmycaptain merged commit 8345781 into toonarmycaptain:development Oct 6, 2020
@toonarmycaptain toonarmycaptain mentioned this pull request Oct 6, 2020
Closed
@ry-v1
Copy link
Contributor Author

ry-v1 commented Oct 6, 2020

Sure... i will add meaningfull commit message.
Any plans to improve the UI using flask-bootstrap.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@toonarmycaptain toonarmycaptain toonarmycaptain approved these changes

Assignees
No one assigned
Labels
hacktoberfest-accepted Accept PR, merge or reject later.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ry-v1 @toonarmycaptain