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

fix issue #101 - 0 padded days, completion date bug, logos for social media buttons #104

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

Conversation

arielzv
Copy link

@arielzv arielzv commented Jun 4, 2018

What issue are you solving? (Give #issuenumber)
#101 , #76

  • Answer here.

How do you solve it? (please write this so a human can understand it - bulletpoints are okay)

  • Answer here.
    this contains 3 issues:
  1. 0 padded days - when formatting the date in "get_completion_date" function, I use "replace" on the return date-string to get rid of all 0 prefix for days.
  2. Completion date is not correct - this one is a bit tricky.
    in prog-o-meter.py, the variable self.completion_date wasn't calculated correctly,

self.completion_date = self.get_completion_date(self.days_remaining-1)

it included the current day, so if for example your goal was set to 1 day (in user.py file), the message on the gui shows:
"... you will be done with this project on TODAY", even before the user clicks 'add a day', after the user clicks 'add a day' the date will not change, thus making this button meaningless (ideally the user will re-open this application every day) and we miss the "achievement" feeling the user can get from adding a day.

so when not including the current day, we get 2 things. 1- the user will get the "achievement" feeling, 2- if you complete the goal and log-on again, you will see the correct date.

self.completion_date = self.get_completion_date(self.days_remaining)

  1. add twitter logo
    I've added logo (and color) to twitter, vk and facebook buttons.
    I created a folder named "resources" in congratulations folder, to hold the logo images.

If there is something specific in your solution, that you would like the reviewer to give feedback on, tell us here:

when I styled the social media buttons, I had to comment out some lines that declared tk-canvas.

the canvas was never used and it just interfered with the screen. please correct me if I was wrong and the canvas is indeed necessary.

If this is your first pull request to the prog-o-meter project, we would really appreciate it if you would fill out this Google form.
This lets us know more about our contributors, and what we can do to make it easier and nicer to contribute to the prog-o-meter project.
Please submit your answer here and put an 'x' in the box below.

  • It is my first pull request to prog-o-meter, and I have answered the survey
  • This is not my first contribution to the prog-o-meter

@arielzv arielzv changed the title fix issue #101 - 0 padded days and completion date bug fix issue #101 - 0 padded days, completion date bug, logos for social media buttons Jun 9, 2018
@arielzv
Copy link
Author

arielzv commented Jun 9, 2018

since all the fixes here are "small" and won't affect the main program, I've combined them to 1 pull request.

Copy link
Collaborator

@lineaba lineaba left a comment

Choose a reason for hiding this comment

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

This is great, I am sorry it has taken me so long to get back to you! If you wanna remove those lines that you currently have commented out (see my comment on the line), then we should be ready to merge.
It also seems to me that the height variable is not used, but you changed the value, so maybe I have just overlooked it. Do we need that variable?

CANVAS_WIDTH = 270
CANVAS_HEIGHT = 160
BTTN_WIDTH = 150
#commented canvas decleration out, I don't understand why it's even here
Copy link
Collaborator

Choose a reason for hiding this comment

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

You are right, that is a mistake, since this window is created as a toplevel window. I plan on standardizing the way we create windows in the future, but for now we just need to remove these lines that you have commented out

@lineaba
Copy link
Collaborator

lineaba commented Jun 21, 2018

Hey, thank you so much for contributing to the prog-o-meter project. I am so sorry it has taken so long to get back to you, my time has been very filled with finals and other practical things! I've left a few comments for you, but mostly this code is ready to merge. Let me know if you have any questions :)

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.

None yet

2 participants