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

Ifad #47

Closed
wants to merge 8 commits into from
Closed

Ifad #47

wants to merge 8 commits into from

Conversation

lleirborras
Copy link
Contributor

Many changes are introduced, most of them are bug fixes. There are some issues around deleting things on the last character of a paragraph, delete selections and things like this that are corrected.

@delambo
Copy link
Member

delambo commented Feb 5, 2013

Thanks for putting this together - from what I can tell, most of these changes look good. Since there are a lot of changes would you mind commenting on some of the commits with details on the bugs that were squashed or features added?

I'm confused by the diff for 2bcf770 and c5d69f4 - I think some code is out of sync with nytimes/ice master. For instance, _getVoidElSelector was changed on master recently and I'm not seeing those changes reflected in the diff. Also for 2bcf770, that diff highlights a large chunk of a delete function, but I'm not sure exactly what changed in that region.

Fill me in as much as you can, please. I would like to work with you and get this into master. Thanks!

@lleirborras
Copy link
Contributor Author

I'll try to fetch master again and see if I can clean the code and comment
you what I did, but this will be tomorrow

2013/2/5 Matthew DeLambo notifications@github.com

Thanks for putting this together - from what I can tell, most of these
changes look good. Since there are a lot of changes would you mind
commenting on some of the commits with details on the bugs that were
squashed or features added?

I'm confused by the diff for 2bcf7702bcf770and
c5d69f4 c5d69f4 - I think some
code is out of sync with nytimes/ice master. For instance,
_getVoidElSelector was changed on master recently and I'm not seeing
those changes reflected in the diff. Also for 2bcf7702bcf770,
that diff highlights a large chunk of a delete function, but I'm not sure
exactly what changed in that region.

Fill me in as much as you can, please. I would like to work with you and
get this into master. Thanks!


Reply to this email directly or view it on GitHubhttps://github.com//pull/47#issuecomment-13136633.

Lleïr Borràs i Metje
http://lleirborras.blogspot.com
http://twitter.com/lleirborras
http://github.com/lleirborras

@johanneswilm
Copy link
Contributor

As said, I followed the commits of @lleirborras closely and merged everything when it was still relevant, except that one line I mentioned.

@lleirborras
Copy link
Contributor Author

That line solves a bug, as it is explained on the comment on the commit

2013/2/6 Johannes Wilm notifications@github.com

As said, I followed the commits of @lleirborrashttps://github.com/lleirborrasclosely and merged everything when it was still relevant, except that one
line I mentioned.


Reply to this email directly or view it on GitHubhttps://github.com//pull/47#issuecomment-13198431.

Lleïr Borràs i Metje
http://lleirborras.blogspot.com
http://twitter.com/lleirborras
http://github.com/lleirborras

@delambo
Copy link
Member

delambo commented Feb 11, 2013

Thanks @lleirborras - we merged your changes in #49.

@delambo delambo closed this Feb 11, 2013
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.

3 participants