Skip to content
This repository was archived by the owner on Oct 2, 2019. It is now read-only.

Refactor features into sub-directives #402

Closed
ProLoser opened this issue Nov 14, 2014 · 4 comments
Closed

Refactor features into sub-directives #402

ProLoser opened this issue Nov 14, 2014 · 4 comments

Comments

@ProLoser
Copy link
Member

As per my comment here: #327 (comment)

I think I would like to see all the features get split out into their own respective directives instead of cramming them all together in one giant one. This way (in the future) if you don't want a feature you can compile your codebase without it (in theory anyway). At the very least it would help make the testing and scope of each directive much easier to deal with and improve the overall API.

How would this work?

uiSelect.directive('uiSelect', function(){
  // doesn't contain multiselect or tagging functionality (theoretically)
});
uiSelect.directive('multiple', function(){
  return {
    restrict: 'A',
    require: [ 'multiple', '?uiSelect'], // making it optional in case some other directive uses a 'tagging' attribute
    controller: function() {
      return {...}; // shareable api goes here
    },
    link: function($scope, $element, $attrs, controllers) {
      var ctrl = controllers[0];
      var uiSelectCtrl = controllers[1];
      if (!uiSelectCtrl) return;
      // add multi-select features here
    }
  };
});
uiSelect.directive('tagging', function(){
  return {
    restrict: 'A',
    require: [ 'tagging', '?uiSelect', '?multiple'], // making it optional in case some other directive uses a 'tagging' attribute
    controller: function() {
      return {...}; // shareable api goes here
    },
    link: function($scope, $element, $attrs, controllers) {
      var ctrl = controllers[0];
      var uiSelectCtrl = controllers[1];
      if (!uiSelectCtrl || !multipleCtrl) return;

      // We can now use uiSelectCtrl to communicate and initialize
      $attrs.$observe('tagging', ...);
      $attrs.$observe('taggingTokens', ...);
      // etc, etc
    }
  };
});
@brianfeister
Copy link

Agreed. No offense to the authors of the original codebase, it's just grown enough that it's time to prune and refactor. 👍

@ProLoser
Copy link
Member Author

I refactored the example to illustrate how we might break out multiple and tagging and then have tagging require both the core uiSelect and multiple features.

@brianfeister as the original inceptor (if that's a word) of this project, I expected this to happen eventually. I'm not saying that it's a standalone directive, merely that repackaging to help break down features better is an awesome way to make this project more manageable since the scope creep is getting a little large. I'd ALSO like to readdress the feature list and see if we can cut out some stupid/useless ones because we can't have this turn into the equivalent of what Select2 is today.

@brianfeister
Copy link

Oh that's great - glad to hear we already have buy-in from the primary product owner ;)

No, I understood your suggestion right away, I thought about it as I was writing #327, but it was clear that I would be touching alot of things that were out of scope.

I also agree about scope creep, though I'm obviously biased about tagging as a feature 😉. I would have to trash my work since I'm using my fork in production for a product I'm building at my day job.

I don't really know the feature set well enough to definitively say what might be on the chopping block.

Lastly, we can call you the mustachio'd man who gave birth to this. Perhaps slightly long-winded, but it has a nice ring 🎉

@dimirc
Copy link
Contributor

dimirc commented Mar 17, 2015

Checking some old issues I just read this one and looks like we were thinking alike on this.
I already start splitting code on #731

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants