-
Notifications
You must be signed in to change notification settings - Fork 17
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
base: master
Are you sure you want to change the base?
Conversation
since all the fixes here are "small" and won't affect the main program, I've combined them to 1 pull request. |
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.
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 |
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 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
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 :) |
What issue are you solving? (Give #issuenumber)
#101 , #76
How do you solve it? (please write this so a human can understand it - bulletpoints are okay)
this contains 3 issues:
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)
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.