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

Add dropdowns for 'parent' and 'child' fields of 'joint' (issue 48 fix) #64

Closed
wants to merge 5 commits into from
Closed

Conversation

pbeeson
Copy link

@pbeeson pbeeson commented Apr 5, 2016

This fixes Issue #48, but builds upon PR #59.

@gavanderhoorn
Copy link
Member

I'll check #70 tomorrow. If that works, could I ask you to rebase so this PR only includes f916110 then?

fyi: for proper attribution, you might want to correct the email address for the committer. Right now it just shows 'Stephen Hart', but github can't link that to the account name.

@pbeeson
Copy link
Author

pbeeson commented Apr 7, 2016

If #70 goes through, I would think this PR would update t only show f916110. If not, we can rebase it. Perhaps it might be best for @swhart115 just to do that and submit a new PR of his own so that it recognizes his credentials.

@gavanderhoorn
Copy link
Member

I've just tested f916110 cherry-picked on top of a merge of #73 and #70, but I couldn't create a valid tree from scratch.

Could I ask you to retest this on top of #73 and then see what happens if you merge in #70?

@pbeeson
Copy link
Author

pbeeson commented Apr 18, 2016

@swhart115 Can you handle this test?

@gavanderhoorn
Copy link
Member

I realise "it doesn't work" wasn't really helpful.

What I observed was that although I could change the parent of a joint, that change didn't seem to be picked-up by the rest of the application (ie: propagated beyond the property itself). Could be that a emit valueChange(..) or similar is missing?

@gavanderhoorn gavanderhoorn changed the title Feature/issue 48 fix Add dropdowns for 'parent' and 'child' fields of 'joint' (issue 48 fix) Apr 18, 2016
@gavanderhoorn
Copy link
Member

@swhart115: I broke this PR (as in: it can't be merged easily), but that is my problem, not yours.

If you follow @pbeeson's suggestion however (just take f916110, rebase it and submit a new PR or forcibly update feature/issue-48-fix), it would be easier.

@pbeeson pbeeson closed this Apr 20, 2016
@pbeeson pbeeson deleted the feature/issue-48-fix branch April 20, 2016 16:06
@pbeeson
Copy link
Author

pbeeson commented Apr 20, 2016

Replaced by PR #78

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