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

added ace.require and advanced options feature to the library. #55

Closed ciel closed 10 years ago

ciel commented 10 years ago

This pull request has 3 major changes.

  1. the issue where settings are loaded twice is fixed.
  2. the ability to support the ace.require function is now implemented, and added to the docs.
  3. the ability to support other options not specifically declared is now available, and added to the docs.

This allows for more configuration, the example I give here is adding autocompletion to the directive without any more javascript code, like this;

<div ui-ace="{
  require: ['ace/ext/language_tools'],
  advanced: {
      enableSnippets: true,
      enableBasicAutocompletion: true,
      enableLiveAutocompletion: true
  }
}"></div>

I am sorry for the previous pull request and how messy it got. This is my first time doing a pull request, so I am still a bit confused. I hope this is more usable.

ciel commented 10 years ago

Oh, that is wonderful! This one actually passes. I'm not sure what else I need to do from this point.

ciel commented 10 years ago

@douglasduteil Has there been any opportunity to look at this and see if this feature is at all desired or useful?

douglasduteil commented 10 years ago

Hi @ciel It looks good. Can you make some test with it ?

ciel commented 10 years ago

Well, to be very honest, I have never done unit tests in java script before. This is only my first or second pull request altogether, and I am pretty new to angular as well.

ciel commented 10 years ago

@douglasduteil

If you wish for some unit tests, can you elaborate on what kind of tests you would like? I've never done this before, so I will have to do some research, but I am willing to try and learn if that is what is necessary to get it included.

douglasduteil commented 10 years ago

Start by looking at the existing tests.

# Grunt will run them
grunt karma:unit
# I added a coverage info with
grunt karma:coverage

Based on the code you're proposing I would like you to test the following case :

uiAce instance options :

  • [ ] should not call "window.ace.require" if there is no "require" options
  • [ ] should call "window.ace.require" if each "require" options
  • [ ] should not call "setOption" if there is no "advanced" options
  • [ ] should use "advanced" options
    (Here you must test if the the passed advance options successfully set the instance ace options but checking the return of ace.getOption(<youAdvencedOption>)).

Is it OK for you @ciel ? BTW I saw a setOptions/getOptions in the ACE changelog It's might be better to use them

ciel commented 10 years ago

Hey, still working on these tests. Sorry it is taking so long.

ciel commented 10 years ago

I am still trying to do this, but having little luck. I'm sorry, I'm just having a very hard time getting into javascript unit testing.

I wrote the first unit test, and it seemed to work for me, but it doesn't work when I commit it to github and I've no idea why. But the second test is causing me a lot of trouble.

The second test is should call "window.ace.require" for each option in "require", but I'm having a difficult time figuring out how to count how many times window.ace.require is called.

  describe('behavior', function () {
    var _ace;

    beforeEach(function () {
      _ace = window.ace;
      spyOn(window.ace, 'require');
    });
    it('should call "window.ace.require" for each option in "require"', function () {
      $compile('<div ui-ace="{ require: [\'ace/ext/language_tools\']}>')(scope);
      scope.$apply();
      expect(_ace.require).toHaveBeenCalled();
    });
  });
ciel commented 10 years ago

Oh! Aha! I got it! Second Unit Test done. Moving on to the next one!

douglasduteil commented 10 years ago

Haha @ciel that's good news. When you finish consider merging all to the initial commit and rename it using those guidelines https://github.com/angular/angular.js/blob/master/CONTRIBUTING.md#-git-commit-guidelines

ciel commented 10 years ago

Alright, I will try. I'm not sure how to do that, though. Should I just create the repository over again?

ciel commented 10 years ago

Okay, I think I got the "merge" thing down. I'm still really lost with github, so I'm very sorry for all the delay and trouble. I'm trying as hard as I can.

ciel commented 10 years ago

@douglasduteil Is there a place where I can see an example of the "rename" you are telling me to do? Where does this go? Is this just text in the commit notes, or is there a specific place I'm supposed to put that kind of information? I'm not understanding what you mean by "rename it".

I used a "rebase", to do what I think you were talking about. It shows all of my different commits as one now, if I'm reading it right. So I believe I only need to figure out this rename you're describing. I read over the document you linked and I'm still pretty lost as to where it goes.

ciel commented 10 years ago

OK! I think I got it all. I did a rebase, and then a commit with the following notes;

feat(directive): require and advanced options

add "require" array for specifying ace.require loads, and add "advanced" object in the directive options for specifying unlisted ace options. This is done to try and make customizing the editor more robust from the directive alone, and possibly eliminate the need for controller code related to the editor.

Is this doing it the correct way?

douglasduteil commented 10 years ago

The note is good but the rebase is bad... Did you do

git fetch && git rebase -i origin/master
douglasduteil commented 10 years ago

You're supposed to merge all you're commit to the fist one you made using squashs.

ciel commented 10 years ago

Alright, this is proving to be very difficult. I've never done any of this before. Should I simply destroy and recreate the repository?

ciel commented 10 years ago

I am sorry, I have done everything I know how to do. I've never used github from a command line, and I still don't even understand what rebasing or squashing is. If the current way it is structured isn't good enough, I need to just destroy the entire repository and pull request and start over. This just isn't working.

douglasduteil commented 10 years ago
![3rmn5q_zpse0e9f7b8](https://cloud.githubusercontent.com/assets/730511/4189055/8a00623c-3778-11e4-9e79-79368f2bb3a7.jpg)
douglasduteil commented 10 years ago

@ciel It's a mess but don't worry I will help :)

douglasduteil commented 10 years ago

I'll be checking you branch on your fork to make it cleaner. I might asked you to much this time... My apologies. Don't destroy the entire repository now. Let me try some stuff before this

ciel commented 10 years ago

You didn't ask too much, I'm just in over my head when it comes to github and unit tests. They are two things I haven't had any real experience with, and getting into them is extremely difficult because they make a lot of assumptions.

I tried a rebase using WebStorm, and I thought that had worked. But then when I tried to run the command you gave me via the Git bash, I got tied up at a VIM editor with no real documentation explaining how to proceed. No commands that I could find in hours of searching. I've used VIM before, but never with github.

ciel commented 10 years ago

I am really sorry for all of the trouble. I was just so excited when I saw that I had done something that might be useful, and was really elated when I finally got the unit tests to work. Then the github stuff came up and I just felt like I was hitting a brick wall.

ciel commented 10 years ago

Even if you help, I'm not going to stop trying to do this squash you mentioned. I really need to learn how to do this. I can't depend on repository admins to be there to bail me out every time.

douglasduteil commented 10 years ago

Sry @ciel I didn't had the time to do it... I'll try it out tonight BTW for the squashing things here is a cool ref.

douglasduteil commented 10 years ago

K @ciel I'm going to close the PR. Can you create a new branch in you repo for your feature ?

ciel commented 10 years ago

Sure, I'll try it out. Thank you again for all of your help. It means a lot to me.

ciel commented 10 years ago

Alright, I made a new branch and everything and submitted the new pull request. Unfortunately the code you gave me didn't work. git co isn't a recognized command. And the squash command you gave me never did what it said it would do. So I ended up just having to delete the entire thing and re-fork the repository, then paste in all of the modified files, and push to a new orphan branch.

douglasduteil commented 10 years ago

Yeah my fault git co is a shortcut for git checkout for me