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

Checklist points and card points update #79

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

Checklist points and card points update #79

wants to merge 9 commits into from

Conversation

onilton
Copy link

@onilton onilton commented Feb 21, 2015

No description provided.

@onilton
Copy link
Author

onilton commented Feb 21, 2015

I used @mgan59 's commit as the base to submit a new pull request that tries to accomplish all the things attempted to fullfill in this pull request #22.

It was heavily inspired by #22 but a LOT of changes were necessary to make it work
with current trunk.

But now you can also:

  • Choose estimated and consumed points in the checklist.
  • Consumed and estimated points are updated in the card title when points are given to checklist items

@SeanColombo
Copy link
Contributor

Screenshot of it in action:
http://screencast.com/t/51apuvwNO2Q

This PR also adds the consumed-points picker to the card title, which was in another PR but which hadn't been merged.

@onilton
Copy link
Author

onilton commented Feb 26, 2015

Consumed-points picker for card title is not in master?

I thought it was:
https://github.com/Q42/TrelloScrum/blob/master/trelloscrum.js#L710

Or am I missing something? :)

@SeanColombo
Copy link
Contributor

Awesome, yeah it is in master. I guess it hadn't been pushed until today? I didn't have it on this computer yesterday, unless I'm mistaken. Cool beans :)

@onilton
Copy link
Author

onilton commented Mar 4, 2015

Any idea on how this pull request could be improved?

mgan59 and others added 9 commits March 5, 2015 00:33
@mgan59 @onilton
inserting point picker into checklist edit-controls
a4d8f9d
@onilton
Use tab for all indents in showPointPickerChecklist
f1d1f19
@onilton
Show estimated and consumed points for showPointPickerChecklist
1f1261a
@onilton
Improving point replacement code
009495a
@onilton
Card points are now updated when points are assigned to checklist items
d41e009 
Heavily inspired by the pull request Q42#22:
Q42#22
But a LOT of changes were necessary to make it work
with current trunk.
@onilton
Improving DOM nodes creation in updateCardTitle
6f9737a
@onilton
Cleanup: Removing development console.log commands
b898469
@onilton
Better name for var: consumedSequence
e570904
@onilton
Replacing really (really) broken for with .each
b5210cb 
This was a mistake of the original code from pull request Q42#22.
We were iterating on pointSeq (Fibonacci numbers). Actually we were
iterating on the indices of this array, so we were stopping at the 9th
item of the list always.
Now we are iterating in all checklists items.
@SeanColombo
Copy link
Contributor

@oniltonmaciel it works really well. I think the only question is conceptual (whether it decreases the simplicity)

@jkaizer: what do you think? If it's potentially too much, maybe we could make it togglable behind a Setting?

@jkaizer
Copy link
Member

jkaizer commented Mar 6, 2015

Awesome work!

I am thinking this should be behind a setting where the default setting disables this functionality.
Is that easy to do?

@mgan59
Copy link
Contributor

mgan59 commented Mar 8, 2015

@oniltonmaciel Been on vacation and traveling, this looks cool. Will pull it down and give it a try. Cheers!

@Kudo
Copy link

Kudo commented Jun 23, 2015

+1 and look forward to seeing this to be published

@SeanColombo
Copy link
Contributor

This still had a few edge-cases if I remember correctly.

Perhaps we should QA & Polish it a bit, then hide it behind a setting?

I just noticed that one of the github Issues for our project, is actually a bug in this PR rather than in the released code: #82

@SeanColombo SeanColombo mentioned this pull request Jan 15, 2016
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@onilton @SeanColombo @jkaizer @mgan59 @Kudo