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

Project is not broken apart properly #784

Open
ProLoser opened this issue Mar 21, 2015 · 2 comments
Open

Project is not broken apart properly #784

ProLoser opened this issue Mar 21, 2015 · 2 comments

Comments

@ProLoser
Copy link
Member

I was reading the ending discussion of #402 and started to poke around the code to see the results of the labor. Although generally it's in the correct direction, I want to bring attention to aspects of the code that can be improved:

Check out these lines:

scope.$watch('sortable', function() {
var sortable = scope.$eval(attrs.sortable);
$select.sortable = sortable !== undefined ? sortable : uiSelectConfig.sortable;
});

These are the only lines pertaining to sortable anywhere outside the sortable directive itself. By having this here, it creates bloat in a file that essentially should have absolutely NO concept of any such sortable-like behavior. These checks are also completely superfluous as the existence of a sortable attribute will cause the loading of the sortable directive which will cause the sortable logic to execute. There shouldn't be any checks, but if there are, they should DEFINITELY not be located in the central repo.

Someone reading through this code has absolutely no idea why this logic is here as there is no other mention of sortable anywhere else in this file. To someone reading JUST this file, it looks like vestigial API.

I would like to bring attention to this so that decoupling can be further improved.

I also find this slightly odd:

require: '^uiSelect',

Instead of require: 'uiSelect' it's require: '^uiSelect' except if my understanding is correct there is no reason this attribute should be on any other element. I throw this out there because again it creates a confusing API and adds to the complexity through which people must deal with to contribute.

All sortable logic should be in the sortable file. All multi logic should be in the multi file, and so on and so forth. Having this bleeding of API and responsibilities completely defeats the purpose of breaking up the project into multiple directives.

@dimirc
Copy link
Contributor

dimirc commented Mar 22, 2015

By no means the refactor started at #731 is complete. But at least the main separation was done that was between the single and multiple mode that I think was urgent since I didn't read the code for some months and when I came back some weeks ago, it was extremely confusing. I wasn't going to wait to be able to do a complete refactor all at once since I don't know when I'll have time to keep doing it so I consider that to start merging was a good idea specially to be able to check issues and possible new PR but under a slightly more organize code.

Sortable and tagging are features that should be abstracted, also to avoid multiple themes is a pending (#186)

@amcdnl
Copy link
Contributor

amcdnl commented Apr 6, 2015

Does using sortable in conjunction with ui-select work? Is there any example(s)?... I'll add to the documentation if so.

I entirely agree with @ProLoser though on how it should not be included, without knowing usage its hard to define how we might extend it to allow users to do this in their own code though.

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

No branches or pull requests

4 participants