Skip to content

- added category plugin that enables adding category for the filters.#455

Closed
cansaner wants to merge 2 commits intomistic100:devfrom
cansaner:dev
Closed

- added category plugin that enables adding category for the filters.#455
cansaner wants to merge 2 commits intomistic100:devfrom
cansaner:dev

Conversation

@cansaner
Copy link

@cansaner cansaner commented Mar 27, 2017

Merge request checklist

  • I read the guidelines for contributing
  • I created my branch from dev and I am issuing the PR to dev
  • Unit tests are OK
  • If it's a new feature, I added the necessary unit tests
  • If it's a new language, I filled the __locale and __author fields

@mistic100
Copy link
Owner

The CI failed on codestyle, run tests locally before pushing.

@mistic100 mistic100 added the feature New feature label Mar 27, 2017
Copy link
Owner

@mistic100 mistic100 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs unit tests

* Read "category" from JSON
*/
this.on('jsonToRule.filter', function(e, json) {
//e.value.category = e.builder.getCategoryById(json.category ? json.category : '-1');
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

need to be implemented


<link rel="stylesheet" href="http://mistic100.github.io/jQuery-QueryBuilder/assets/flags/flags.css">
<style>
.flag {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please do not change the indentation of the whole file.

this.categories = this.settings.categories;

// CHECK ALL CATEGORIES
this.categories = this.checkCategories(this.categories);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

merge the two lines

// keep a pointer to the original addRule method
var originaAddRule = QueryBuilder.prototype.addRule;

QueryBuilder.extend({
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not ok, you must find a way the call the original method (but putting the new code before or after) or use existing events, or add new events where needed.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same for all overwritten methods

isFilterOKForCategory: function(filterId, categoryId) {
var filtersOfCategory = this.getFiltersForCategoryId(categoryId);

for (var i = 0; i < filtersOfCategory.length; i++) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can make usage of filtersOfCategory.some method

getFiltersForCategoryId: function(categoryId) {
var filtersOfCategory = [];
var category = this.getCategoryById(categoryId);
this.filters.forEach(function(filter, i) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

useless double foreach : loop over category.filters and use getFilterById

* @return {boolean} true if filter belongs to that category
*/
isFilterOKForCategory: function(filterId, categoryId) {
var filtersOfCategory = this.getFiltersForCategoryId(categoryId);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

useless access to getFiltersForCategoryId: use getCategoryById and loop over its filters

@mistic100
Copy link
Owner

@cansaner there are two much breakable code (overriden methods) for this to be merged.

You are free to release this as a separated standalone plugin in it's own repository (I can help you to make it standalone if you want).

@mistic100 mistic100 closed this Aug 7, 2017
@cansaner
Copy link
Author

cansaner commented Aug 7, 2017

@mistic100 I will definitely focus on making it standalone. My thesis statement is pushing me hard these days. I ll hopefully focus back on October I guess... Thanks a lot in advance

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feature New feature

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants