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

In-place directive manipulation support #46

Closed
wants to merge 2 commits into from

Conversation

miohtama
Copy link
Contributor

Add support for cases where directive handler uses jQuery to directly manipulate DOM in-place, instead of returning text or HTML snippet.

In complex manipulation cases this may be

  • Faster
  • Cleaner
  • Functionally difficult to achieve

The change itself is that if directive function returns null then don't try to run text or html replacement on this null value.

@pyykkis
Copy link
Contributor

pyykkis commented Apr 22, 2012

How about just

directives =
  person: (elem, i) ->
    elem = jQuery elem
    elem.attr "foobar", "foo"
    elem.text "daa"
    {}

I didn't test it, but as far as I see, it should work with current implementation?

Of course, the pull request would make things easier with javascript, as one could omit the last line.

@miohtama
Copy link
Contributor Author

Thinking outside CoffeeScript box:

  • Javascript methods do not need to do explicit return
  • undefined return is automatic

... so this is much more useful in JS side of the things and users do not return magical {}.

It is mostly what "users would think how API behaves" and thus will save lots of people from gotcha.

@pyykkis
Copy link
Contributor

pyykkis commented Apr 23, 2012

Fair enough, I'll merge to master :)

Before that, a bit of cleanup is needed. The pull request contains implementation also for the object-like directives, which is a separate topic.

@miohtama
Copy link
Contributor Author

Looks like Github pull requests somehow picksk up the latest commits even if they are not on available on the moment I made the pull requests. Github > me :(

Probably the correct use is to make only one pull request per branch?

@pyykkis
Copy link
Contributor

pyykkis commented Apr 24, 2012

Yes, that's correct. Easiest way is to make a pull request is from a dedicated feature branch (which is kept up-to-date with upstream/master).

http://help.github.com/send-pull-requests/

@miohtama
Copy link
Contributor Author

Used git create branch from commit and then re-created pull request to get correct commit range:

http://stackoverflow.com/questions/2816715/branch-from-a-previous-commit-using-git

Apparently Github pulls always the whole branch in the request and you cannot set commit range (even though it says commit range in the Github user interface). Please see the new pull request, closing this one.

@miohtama miohtama closed this Apr 24, 2012
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