angular-ui / ui-ace

This directive allows you to add ACE editor elements.
http://angular-ui.github.io/ui-ace
MIT License
578 stars 172 forks source link

Data binding support added for ace options and JSDoc blocks added #36

Closed slopjong closed 10 years ago

slopjong commented 10 years ago

With my changes there's more flexibility on higher application layers. If you'd create a directive based on ui-ace this looked as follows:

In a HTML file:

<div je-ace></div>

In the corresponding directive:

angular.module("jsoneditor", ['ui.ace'])
  .directive("jeAce", function($compile) {
    return {
      restrict: 'EA',
      template: '<div class="je-ace" ui-ace="$parent.jsoneditor.ace.options" ng-model="$parent.jsoneditor.json"></div>',
      replace: true,
      transclude: true
    };
  });

Somewhere in the controller of a higher application layer:

$scope.jsoneditor = {
          json: '{test: "tasdfasdf"}',
          ace: {
            options: {
              mode: 'xml',
              onChange: function() {
                console.log('onChange1');
              }
            }
          }
        };

        // we simulate a change
        $timeout(
          (function(scope){
            return function(){
              scope.jsoneditor.ace.options.mode = 'json';
              scope.jsoneditor.ace.options.onChange = function() {
                console.log('onChange2');
              };
            }
          }($scope))
        , 3000
        );

The ace options are smoothly passed to the ui-ace directive being watched after linking.

The old way still works with the json directly passed.

angular.module("jsoneditor", ['ui.ace'])
  .directive("jeAce", function($compile) {
    return {
      restrict: 'EA',
      template: '<div class="je-ace" ui-ace="{mode: \'json\'}" ng-model="$parent.jsoneditor.json"></div>',
      replace: true,
      transclude: true
    };
  });

The tests were successful.

slopjong commented 10 years ago

@aruszkowski @jessezhang91 I made some changes which are incompatible to your pull requests. I pre-merged your changes manually without using git-merge because it was faster this way. Though you won't appear as a contributor unless @douglasduteil accepts yours first and applies a take-theirs merge on this pull request.

@douglasduteil if there are chances that you accept this pull request, could we get it to the upstream soon? Future pull requests lead to additional work.

douglasduteil commented 10 years ago

Hi @slopjong sorry to not been there those days... I'll trust you for now

slopjong commented 10 years ago

@douglasduteil thanks.

If things break, I'll feel responsible to fix it. Did you have a look at the code? Are you comfortable with the doc blocks, the factory methods and what people added to the directive?

If so, I'd take some (future) feature requests and get them done if they're reasonable.

I also didn't generate any documentation from the code so I don't know if the output looks nice at all. Some documentation, however, is better than nothing.

douglasduteil commented 10 years ago

@slopjong It's cool. :) The only thing that I don't like is PR made form master branches... But the repo needed those features

slopjong commented 10 years ago

There are only the bower, gh-pages and master branches but no develop. How should I have proceeded, just in case I create more PRs?

douglasduteil commented 10 years ago

Just use another branch based on the origin/master and make your pr from it http://stackoverflow.com/questions/14680711/how-to-do-a-github-pull-request#answer-14681796

slopjong commented 10 years ago

Thx, I'll follow that guide next time.